They were not being split properly.
Details
- Reviewers
dfaure - Group Reviewers
Frameworks - Commits
- R237:5f340fc84d89: Fix issue when reading path lists
Tests pass, including the new one.
Diff Detail
- Repository
- R237 KConfig
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 1563 Build 1581: arc lint + arc unit
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 ↗ | (On Diff #37886) | 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 ↗ | (On Diff #37886) | 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. |
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?
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.
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)
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.
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).