They were not being split properly.
Details
Details
- Reviewers
dfaure - Group Reviewers
Frameworks - Commits
- R237:5f340fc84d89: Fix issue when reading path lists
Tests pass, including the new one.
Diff Detail
Diff Detail
- Repository
- R237 KConfig
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| src/core/kconfiggroup.cpp | ||
|---|---|---|
| 176 ↗ | (On Diff #37886) | It should be p < data.size() && data[p] == '\\' or you will access not owned memory. Better is to cache data.size() in variable then to use it everywhere. |
| autotests/kconfigtest.cpp | ||
|---|---|---|
| 524–526 | You broke the "no EOL" testcase. | |
| src/core/kconfiggroup.cpp | ||
| 163 ↗ | (On Diff #37886) | I don't understand why this needs a vector. Isn't this only about consecutive backslashes? |
| src/core/kconfigini.cpp | ||
| 797 | Did you mean '\,' here? | |
| src/core/kconfiggroup.cpp | ||
|---|---|---|
| 163 ↗ | (On Diff #37886) | We're extracting the whole string and then removing the escapes, we need to remember them all. |
Comment Actions
I don't really understand why we can't just skip the escapes as we go along, as most parsers do, for the sake of efficiency. This is already a state-machine based parser so it should be easy to integrate that in, no?
Comment Actions
You mean we could integrate it in KConfigIni itself?
Currently values are always stored as QString, it would need a refactoring to allow to store values in different types.
Comment Actions
No, I don't think I mean that.
The code in kconfiggroup is already iterating over one character at a time and handling escapes and .... wait, what are we fixing here exactly?
I applied your patch, reverted kconfiggroup.cpp, and the new unittest still passes.
PASS : KConfigTest::testPath()
Could it be that your change in kconfigini.cpp is enough to fix the bug?
(Otherwise it means the unittest isn't comprehensive enough)
Comment Actions
Correct, I wanted to fix all the allocations as the commit message says, then I realised it wasn't even working well when I added the unit test, so I fixed that too.
The problem with the allocation is that it's allocating for the remaining part of the field and freeing again the rest every time we read each element of the list. This showed up in heaptrack for Discover.
Comment Actions
Can we split this into two commits then? The bugfix (which certainly seems fine to me), and the optimization (which is separate and needs to be measured for increased CPU usage, and alternative solutions like QStringRef).