D14162: Figure out the escaped path list on kconfig

apol updated this revision to Diff 42504. apol added a comment. Removed optimisation REPOSITORY R237 KConfig CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14162?vs=39182&id=42504 BRANCH master REVISION DETAIL https://phabricator.kde.org/D14162 AFFECTED FILES autotests/kc

D14162: Figure out the escaped path list on kconfig

apol added a comment. ping? REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D14162 To: apol, #frameworks, dfaure Cc: dfaure, anthonyfieroni, kde-frameworks-devel, michaelh, ngraham, bruns

D14162: Figure out the escaped path list on kconfig

dfaure added a comment. 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). REPOSITORY R237 KConfig REVISION DETAIL h

D14162: Figure out the escaped path list on kconfig

apol added a comment. 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 fre

D14162: Figure out the escaped path list on kconfig

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. 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?

D14162: Figure out the escaped path list on kconfig

apol added a comment. In D14162#304826 , @dfaure wrote: > 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 e

D14162: Figure out the escaped path list on kconfig

dfaure added a comment. 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? REPOSITORY R237 KConfig REVISION DETAIL https:

D14162: Figure out the escaped path list on kconfig

apol updated this revision to Diff 39182. apol added a comment. Addressed issues by David and Anthony REPOSITORY R237 KConfig CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14162?vs=37886&id=39182 BRANCH master REVISION DETAIL https://phabricator.kde.org/D14162 AFFECTED FIL

D14162: Figure out the escaped path list on kconfig

apol marked 3 inline comments as done. apol added inline comments. INLINE COMMENTS > dfaure wrote in kconfiggroup.cpp:163 > I don't understand why this needs a vector. Isn't this only about consecutive > backslashes? > Wouldn't a counter be enough? We're extracting the whole string and then rem

D14162: Figure out the escaped path list on kconfig

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kconfigtest.cpp:524 > << "hostname[$e]=$(hostname)" << endl > -<< "noeol=foo"; // no EOL > +<< "noeol=foo" << endl // no E

D14162: Figure out the escaped path list on kconfig

anthonyfieroni added inline comments. INLINE COMMENTS > kconfiggroup.cpp:176 > +last = p + 1; > +} else if (data[p] == QLatin1Char('\\')) { > +escapedLast = true; It should be p < data.size() && data[p] == '\\' or you will access not owned memory. Better is to

D14162: Figure out the escaped path list on kconfig

apol added a comment. ping? REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D14162 To: apol, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14162: Figure out the escaped path list on kconfig

apol added a comment. ping REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D14162 To: apol, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14162: Figure out the escaped path list on kconfig

apol created this revision. apol added a reviewer: Frameworks. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: kde-frameworks-devel. apol requested review of this revision. REVISION SUMMARY Upon close look I realised that there was a lot of allocati