Details
- Reviewers
davidedmundson - Commits
- R246:bf8f3067fd3a: Loader: Avoid Q_FOREACH
Diff Detail
- Repository
- R246 Sonnet
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
@davidedmundson Wow, that was fast :-)
I have now done a search for more Q_FOREACH/foreach. Please let me know if you want each as own diff or all in once.
data/parsetrigrams.cpp src/core/guesslanguage.cpp src/plugins/hspell/hspelldict.cpp src/plugins/hunspell/hunspellclient.cpp src/plugins/voikko/voikkodict.cpp
| src/core/loader.cpp | ||
|---|---|---|
| 273 | will languages() detach ? | |
.I have now done a search for more Q_FOREACH/foreach. Please let me know if you want each as own diff or all in once
Can be all at once, but still go via review. There are some foreach / for differences and pitfalls to be wary of.
| src/core/loader.cpp | ||
|---|---|---|
| 273 | Probably, yes. But Qt docu always says, "thanks to implicit sharing copying a container is very fast" Is copy previous into a const var a benefit (as I have seen recently)? Don't think so. Or would iterate by index avoid that? (It's a QStringList) I don't know. Guess the problem remains. You need to keep a copy. I think I read something like, "Don't worry when using a function return value", but can't find it anymore. So, perhaps it was my conclusion from here: http://doc.qt.io/qt-5/qtglobal.html#qAsConst Reads similar to me: https://www.kdab.com/goodbye-q_foreach/ Let me know what you like to see, will do it. | |
| src/core/loader.cpp | ||
|---|---|---|
| 273 |
That's mixing up docs. foreach() always does a const copy of the list. This is a cheap implicitly shared copy. for() does not In this specific case language() returns a new QStringList (.keys() generates a new list) so I /think/ it is fine, but it's definitely ambiguous. A qAsConst would make sense here. | |
| src/core/loader.cpp | ||
|---|---|---|
| 273 |
In which way? This is not working: for (const QString &langCode : qAsConst(languages())) So you like to see this? const QStringList constLanguages = languages();
for (const QString &langCode : constLanguages) { | |
@davidedmundson
May I ask how does this here will progress? You seemed to request a change which was not clear to me, but it's anyway "green flagged".
Is your full name really "loh tar", spelled with lower case letters? It looked strange on the list of names of all committers.
Well, neither of that. My "online me" is loh.tar but that was rejected by Phabricator, so the dot had to go. Lastly used ngraham uppercase letters, what looks even more strange to me :-)
Should you intend to commit this, thanks in advance! May you look at D17055 too?
I do not know if there is a written rule that we require commits with real names (for legal reasons), but I cannot remember we didn't ask contributors for their real name for contributions.
..but I cannot remember we didn't ask contributors for their real name for contributions.
Um, what? I was asked. I hope it's ok as it is.
@cfeck wrote:
It looked strange on the list of names of all committers.
A look at your page indicates that you are doing some public relations? Should you copy&paste some data from an auto generated list, feel free to remove me.
We started to accept his patches for other frameworks, I think there is no harm in doing so, here, too. Real name or not.