Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace QLatin1String with QStringLiteral #769

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iamthesenate1
Copy link
Contributor

@iamthesenate1 iamthesenate1 commented Sep 29, 2021

Output of clazy with CLAZY_CHECKS="qstring-allocations".

I've seen that opinions are divided about which one is better.
From my point of view, if this PR is merged, good. If not, also good.

@tsujan
Copy link
Member

tsujan commented Oct 1, 2021

Personally, I think QStringLiteral is better in these places.

I've seen that opinions are divided about which one is better.

Yes, I've noticed it too.

Contrary to what Qt doc implies, I've seen no tangible gain in using either of them instead of QString::QString(const char *str) for (tens of) thousands of strings (while I've seen big gains in quite different things, about which Qt doc is silent).

Copy link
Member

@tsujan tsujan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought and apart from personal opinions (including mine), if we want to follow Qt doc, we shouldn't change all of these cases. According to Qt doc:

Note: QLatin1String can still be more efficient than QStringLiteral when the string is passed
to a function that has an overload taking QLatin1String and this overload avoids conversion to QString.

QString::operator=, QString::operator== and QString::operator+= are among those overloads.

But, for example, QSettings::setValue doesn't have such an overload and QStringLiteral seems a better choice for it.

All in all, changes may be made but more carefully.

@iamthesenate1
Copy link
Contributor Author

I might check for such cases later. For now, I'm very busy and I regard these changes as a pretty low priority.

@tsujan
Copy link
Member

tsujan commented Oct 4, 2021

Take your time. There's no rush at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants