Move newItem to private method in KConfigSourceGenerator
Needs ReviewPublic

Authored by tcanabrava on Jan 23 2020, 2:31 PM.

Details

Reviewers
ervin
dfaure
Summary

This is only used here, should only be exposed here.

Test Plan

Run unittests

Diff Detail

Repository
R237 KConfig
Branch
simplify_newEntry
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21801
Build 21819: arc lint + arc unit
tcanabrava created this revision.Jan 23 2020, 2:31 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 23 2020, 2:31 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
tcanabrava requested review of this revision.Jan 23 2020, 2:31 PM
ervin added inline comments.Jan 28 2020, 5:58 PM
src/kconfig_compiler/KConfigSourceGenerator.cpp
659

Since we're here, what about fixing the coding style? (same thing for the declaration)

kossebau added a subscriber: kossebau.EditedJan 28 2020, 6:05 PM

And more while at it: for latin1 strings used in concatenation it is recommended to use QLatin1String and for single chars QLatin1Char. QStringLiteral is more expensive here, and QStringBuilder & QString has proper overloads for the concatenations. So you want to keep the QLatin1Strings and rather turn those for single chars to QLatin1Char.

Edit: and same for == comparisons

There is indeed a QString overload for concatenating QLatin1String, but it will have to be converted char-by-char (from 8 bits to 16 bits), so isn't it faster to concatenate QStringLiterals?

There is indeed a QString overload for concatenating QLatin1String, but it will have to be converted char-by-char (from 8 bits to 16 bits), so isn't it faster to concatenate QStringLiterals?

I have never seen numbers, but any time I asked someone who might have seen numbers, they told me: QStringLiteral only for final strings not further modified or given to operations. Sadly cannot find references now, but I asked often the recent years.
Looking at the code, the qt_from_latin1 helper method might be as efficient on most processors as the memcpy is perhaps, and making up for any other overhead the QStringLiteral-generated code has, perhaps atomic stuff around the generated QString instance is expensive compared to the copy logic?

But ready to hear from real Qt experts once more, perhaps with numbers this time :)

There is indeed a QString overload for concatenating QLatin1String, but it will have to be converted char-by-char (from 8 bits to 16 bits), so isn't it faster to concatenate QStringLiterals?

I have never seen numbers, but any time I asked someone who might have seen numbers, they told me: QStringLiteral only for final strings not further modified or given to operations. Sadly cannot find references now, but I asked often the recent years.
Looking at the code, the qt_from_latin1 helper method might be as efficient on most processors as the memcpy is perhaps, and making up for any other overhead the QStringLiteral-generated code has, perhaps atomic stuff around the generated QString instance is expensive compared to the copy logic?

But ready to hear from real Qt experts once more, perhaps with numbers this time :)

I checked here with a really big xml file and the difference seems negligible. ( QStringLiteral 0.0s and QLatin1String 0.0s ) - I'd use QStringLiteral just for the sake of consistency across the codebase, tough

tcanabrava updated this revision to Diff 74600.Jan 29 2020, 5:38 PM
  • Fix code style

Also compared binary size? ;) You still want to use QLatin1Char with single characters, that is less undisputed agreed upon as better choice :)