optimize CMakeBuildDirChooser::buildDirSettings()
Needs RevisionPublic

Authored by dakon on Fri, Feb 8, 4:58 PM.



-if we have a match for a key, no other key can match
-match the start of a new line as QByteArray instead of QString, most lines will
not match and all keys are plain ASCII, this saves a lot of needless
conversions and memory allocations
-modify the original line instead of doing a copy, the original one will not
be used again anyway.

Test Plan

Open and build a CMake-based project.

Diff Detail

R32 KDevelop
No Linters Available
No Unit Test Coverage
Build Status
Buildable 8039
Build 8057: arc lint + arc unit
dakon created this revision.Fri, Feb 8, 4:58 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptFri, Feb 8, 4:58 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
dakon requested review of this revision.Fri, Feb 8, 4:58 PM

I wonder if that's actually faster. Here we are creating intermediary copies of the QByteArray only to convert to QString.

The QByteArray was already created the same way, now the intermediate object is just cached. But even if that did not change anything it reduces the number of utf8 conversion as only those lines that actually match anything will be converted.

in exchange of much harder to read code.
Was it ever a bottleneck for you?

dakon added a comment.Sun, Feb 10, 4:30 PM

No, I was just looking at it. I could remove the QByteArray change and reduce it to online the x.remove() and "else if" parts.

mwolff requested changes to this revision.Fri, Feb 15, 9:51 AM
mwolff added a subscriber: mwolff.

using byte arrays is OK imo, but please cleanup the code overall by using a lambda instead of repeating the same thing over and over again


const auto rawLine = ...


introduce a helper lambda:

auto match = [&rawLine](const QByteArray& prefix, QString* target) -> bool
    if (line.startsWith(prefix) {
        *target = QString::formUtf8(line.constData() + prefix.size(), line.size() - prefix.size());
        return true;
    return false;

use it:

if (match(srcLine, &srcDir) || match(installLine, &installDir) || match(buildLine, &buildType)) {
This revision now requires changes to proceed.Fri, Feb 15, 9:51 AM