Save few string allocations
AbandonedPublic

Authored by apol on Jul 30 2018, 11:50 PM.

Details

Reviewers
ilic
Group Reviewers
Frameworks
Summary

We are not using them on their own, so it's doable. Not a big deal
anyway.

Test Plan

Tests pass

Diff Detail

Repository
R249 KI18n
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1386
Build 1404: arc lint + arc unit
apol created this revision.Jul 30 2018, 11:50 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 30 2018, 11:50 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
apol requested review of this revision.Jul 30 2018, 11:50 PM
pino added a subscriber: pino.Aug 1 2018, 6:44 AM

TBH I had the idea of making splitLocale() public, since it was a public API in KLocale. I still see a couple of users of it, and in the past I remember people working around the lack of splitLocale() by doing the same job on their own, when porting away from kdelibs4support.

With that in mind, IMHO using QStringRef for a public API still seems odd to me, especially that if you really care about any part split, then most probably you will pass to other APIs that use QString (sort of nullifying the gain from using QStringRef).

I saw too many cases in kdelibs 4.x applications using patterns like:

QString language, dummy;
KLocale::splitLocale(locale, language, dummy, dumy, dummy);

Because of that, a better API would be to pass QString* for the split parts, so you can decide what actually you want as results -- e.g.:

QString language;
KLocalizedString::splitLocale(locale, &language, nullptr, nullptr, nullptr);
dfaure added a subscriber: dfaure.Aug 1 2018, 9:22 AM

Has this been measured to actually reduce the number of string allocations? (e.g. using heaptrack)?

If so, I'm fine with it. If a public API is needed, then indeed that will have different considerations -- it might want to them wrap the internal QStringRef-based function.

apol abandoned this revision.Aug 1 2018, 10:17 PM

Probably not worth it if it needs to become public.