D23902: [KCoreDirLister] replace deprecated foreach with range-for

ahmadsamir added a comment. In D23902#542046 , @dfaure wrote: > In D23902#542037 , @ahmadsamir wrote: > > > In D23902#542026 , @dfaure wrote: > > > >

D23902: [KCoreDirLister] replace deprecated foreach with range-for

dfaure added a comment. In D23902#542037 , @ahmadsamir wrote: > In D23902#542026 , @dfaure wrote: > > > For the record, JFBastien was actually wrong. Calling .begin() on a const return value does

D23902: [KCoreDirLister] replace deprecated foreach with range-for

ahmadsamir added a comment. In D23902#542026 , @dfaure wrote: > For the record, JFBastien was actually wrong. Calling .begin() on a const return value does call the const overload. Testcase http://www.davidfaure.fr/kde/const_retval.cpp

D23902: [KCoreDirLister] replace deprecated foreach with range-for

dfaure added a comment. For the record, JFBastien was actually wrong. Calling .begin() on a const return value does call the const overload. Testcase http://www.davidfaure.fr/kde/const_retval.cpp But returning const QList would inhibit move semantics, e.g. `QList mylist = foo();`

D23902: [KCoreDirLister] replace deprecated foreach with range-for

ahmadsamir added inline comments. INLINE COMMENTS > dfaure wrote in kcoredirlister.cpp:1044 > Actually, I was wrong. > I talked to the head of the LLVM/clang development team at Apple and he told > me that returning a const value has zero effect whatsoever. I.e. in the > version of your patch

D23902: [KCoreDirLister] replace deprecated foreach with range-for

dfaure added inline comments. INLINE COMMENTS > ahmadsamir wrote in kcoredirlister.cpp:1044 > Noted. Actually, I was wrong. I talked to the head of the LLVM/clang development team at Apple and he told me that returning a const value has zero effect whatsoever. I.e. in the version of your

D23902: [KCoreDirLister] replace deprecated foreach with range-for

dfaure added a comment. Too late, what's pushed is pushed (no force-push allowed, it would break existing checkouts with non-pushed work on top). No big deal. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D23902 To: ahmadsamir, kde-frameworks-devel, dfaure Cc:

D23902: [KCoreDirLister] replace deprecated foreach with range-for

ahmadsamir added a comment. I am sorry, I forgot to use --verbatim, please remove the line about directoriesForCanonicalPath() returning const from the commit message :/ REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D23902 To: ahmadsamir, kde-frameworks-devel, dfaure

D23902: [KCoreDirLister] replace deprecated foreach with range-for

This revision was automatically updated to reflect the committed changes. Closed by commit R241:3c955240e87b: [KCoreDirLister] replace deprecated foreach with range-for (authored by ahmadsamir, committed by dfaure). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D23902: [KCoreDirLister] replace deprecated foreach with range-for

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH ahmad/foreach (branched from master) REVISION DETAIL https://phabricator.kde.org/D23902 To: ahmadsamir, kde-frameworks-devel, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2,

D23902: [KCoreDirLister] replace deprecated foreach with range-for

ahmadsamir updated this revision to Diff 66393. ahmadsamir marked 2 inline comments as done. ahmadsamir added a comment. Fix patch REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23902?vs=66073=66393 BRANCH ahmad/foreach (branched from master) REVISION

D23902: [KCoreDirLister] replace deprecated foreach with range-for

dfaure added a comment. Don't confuse the two rules of porting to range-for (for Qt containers) 1. The container should be const, to avoid paying for a detach. So either a local const var, or qAsConst() over a non-const var (but never a temporary returned by a function!) 2. The

D23902: [KCoreDirLister] replace deprecated foreach with range-for

ahmadsamir marked 10 inline comments as done. ahmadsamir added a comment. I think I got all the bits I missed before (sorry about the mess). But I'll sleep on it anyway, will submit in the morning (usually I find mistakes in my code when I look at it again in the morning). Thanks.

D23902: [KCoreDirLister] replace deprecated foreach with range-for

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kcoredirlister.cpp:833 > qCDebug(KIO_CORE_DIRLISTER) << urlDir; // output urls, not qstrings, > since they might contain a password > -Q_FOREACH

D23902: [KCoreDirLister] replace deprecated foreach with range-for

ahmadsamir added inline comments. INLINE COMMENTS > dfaure wrote in kcoredirlister.cpp:1044 > It's generally considered bad practice to return a const value, because the > caller makes a copy anyway, so this doesn't guarantee anything. And it > removes the benefits of rvalues that can be

D23902: [KCoreDirLister] replace deprecated foreach with range-for

ahmadsamir updated this revision to Diff 66073. ahmadsamir marked 7 inline comments as done. ahmadsamir retitled this revision from "[KCoreDirLister] replace foreach with range-for" to "[KCoreDirLister] replace deprecated foreach with range-for". ahmadsamir added a comment. Fix stuff