D27965: [KPasswdServer] replace foreach with range/index-based for

This revision was automatically updated to reflect the committed changes. Closed by commit R241:018f223692de: [KPasswdServer] replace foreach with range/index-based for (authored by ahmadsamir). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27965?vs=77554=77648

D27965: [KPasswdServer] replace foreach with range/index-based for

dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH l-kpasswdserver (branched from master) REVISION DETAIL https://phabricator.kde.org/D27965 To: ahmadsamir, #frameworks, dfaure, meven Cc: apol, kde-frameworks-devel, LeGast00n,

D27965: [KPasswdServer] replace foreach with range/index-based for

ahmadsamir updated this revision to Diff 77554. ahmadsamir added a comment. QList::erase() invalidates the iterators REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27965?vs=77477=77554 BRANCH l-kpasswdserver (branched from master) REVISION DETAIL

D27965: [KPasswdServer] replace foreach with range/index-based for

dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kpasswdserver.cpp:201 > > bool KPasswdServer::hasPendingQuery(const QString , const KIO::AuthInfo > ) > { I wonder why the whole method isn't const >

D27965: [KPasswdServer] replace foreach with range/index-based for

ahmadsamir updated this revision to Diff 77477. ahmadsamir added a comment. - I missed one foreach before - Use erase() instead of remove(), safer to work on QList iterators REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27965?vs=77339=77477 BRANCH

D27965: [KPasswdServer] replace foreach with range/index-based for

dfaure requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D27965 To: ahmadsamir, #frameworks, dfaure, meven Cc: apol, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27965: [KPasswdServer] replace foreach with range/index-based for

dfaure added a comment. In D27965#625676 , @ahmadsamir wrote: > In D27965#625526 , @apol wrote: > > > Having the iterated value change under the hood will eventually break. I'd suggest preferring

D27965: [KPasswdServer] replace foreach with range/index-based for

ahmadsamir added a comment. In D27965#625526 , @apol wrote: > Having the iterated value change under the hood will eventually break. I'd suggest preferring QList::erase to QList::removeOne. Actually, the code also uses 'delete current',

D27965: [KPasswdServer] replace foreach with range/index-based for

apol added a comment. Having the iterated value change under the hood will eventually break. I'd suggest preferring QList::erase to QList::removeOne. INLINE COMMENTS > kpasswdserver.cpp:648 > + static_cast(time(nullptr)) > current->expireTime) { >

D27965: [KPasswdServer] replace foreach with range/index-based for

meven added a comment. In D27965#625381 , @apol wrote: > Looks good to me, I wonder why you turned some to for+iterators. Those modified the list as they iterated through the list : `authList->removeOne(current);` REPOSITORY R241 KIO

D27965: [KPasswdServer] replace foreach with range/index-based for

apol added a comment. Looks good to me, I wonder why you turned some to for+iterators. REPOSITORY R241 KIO BRANCH l-kpasswdserver (branched from master) REVISION DETAIL https://phabricator.kde.org/D27965 To: ahmadsamir, #frameworks, dfaure, meven Cc: apol, kde-frameworks-devel,

D27965: [KPasswdServer] replace foreach with range/index-based for

ahmadsamir added a comment. In D27965#625234 , @meven wrote: > Please wait for a second review, I am not authoritative here. OK, thanks. REPOSITORY R241 KIO BRANCH l-kpasswdserver (branched from master) REVISION DETAIL

D27965: [KPasswdServer] replace foreach with range/index-based for

meven added a comment. Please wait for a second review, I am not authoritative here. REPOSITORY R241 KIO BRANCH l-kpasswdserver (branched from master) REVISION DETAIL https://phabricator.kde.org/D27965 To: ahmadsamir, #frameworks, dfaure, meven Cc: kde-frameworks-devel, LeGast00n,

D27965: [KPasswdServer] replace foreach with range/index-based for

meven accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO BRANCH l-kpasswdserver (branched from master) REVISION DETAIL https://phabricator.kde.org/D27965 To: ahmadsamir, #frameworks, dfaure, meven Cc: kde-frameworks-devel, LeGast00n, cblack,

D27965: [KPasswdServer] replace foreach with range/index-based for

ahmadsamir created this revision. ahmadsamir added reviewers: Frameworks, dfaure, meven. ahmadsamir added a project: Frameworks. ahmadsamir requested review of this revision. TEST PLAN make && ctest REPOSITORY R241 KIO BRANCH l-kpasswdserver (branched from master) REVISION DETAIL