D17028: Loader: Avoid Q_FOREACH

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> loh.tar wrote in 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.

> Probably, yes. But Qt docu always says, "thanks to implicit sharing copying a container is very fast"

That's mixing up docs.

foreach() always does a const copy of the list. This is a cheap implicitly shared copy.
when we loop we're never detaching this list because it's const.

for() does not
So when we loop if we call begin() we might get the non-const version. If this list is used elsewhere that means we detach and that causes a deep-copy. This is expensive.

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.

REPOSITORY
  R246 Sonnet

REVISION DETAIL
  https://phabricator.kde.org/D17028

To: loh.tar, davidedmundson
Cc: smartins, kde-frameworks-devel, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20181120/0a97ea91/attachment.html>