----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125750/#review87265 -----------------------------------------------------------
Ship it! There's probably a way of avoinding the allocation and only iterating over the line once but it's a big improvement like this anyway. Iterating over a few more bytes that are already in the cache shouldn't be noticable, whereas saving one call to malloc() per .desktop file line is a big improvement. Also calling contains() it makes the code a lot easier to understand and maintain. I don't really know the details of how plasma plugins are handled but for normal shared libraries using .json instead of .desktop is the way to go. That's why there's the transitional comment in the source. If plasma plugins have metadata.desktop files, using metadata.json instead should (hopefully) just work. However, I haven't looked into the plasma code so I can't be certain. src/lib/plugin/desktopfileparser.cpp (line 97) <https://git.reviewboard.kde.org/r/125750/#comment59944> This should be '\' Looks I didn't test escaped values if the tests still pass... - Alex Richardson On Oct. 22, 2015, 2:20 p.m., Aleix Pol Gonzalez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/125750/ > ----------------------------------------------------------- > > (Updated Oct. 22, 2015, 2:20 p.m.) > > > Review request for KDE Frameworks and Sebastian Kügler. > > > Repository: kcoreaddons > > > Description > ------- > > I know this could be done so much better and there's a comment saying it's > transitional (heh), but this happens to be called a ton of times on every > plasma boot through [1]. > Actually, why is it transitional? Are we supposed to move from > metadata.desktop to metadata.json? Or is it because we should all be using > `kpackagetool5 --generate-index`? > > [1] KPluginMetaData::loadFromDesktopFile(QString const&, QStringList const&) > > > Diffs > ----- > > src/lib/plugin/desktopfileparser.cpp 7a4556a > > Diff: https://git.reviewboard.kde.org/r/125750/diff/ > > > Testing > ------- > > Tests pass, plasma still boots. > > > Thanks, > > Aleix Pol Gonzalez > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
