Issues/2219/deprecate foreach #2272

snake66 · 2020-08-15T14:14:28Z

A different take on issue #2219 replacing previous PR #2240.

As #2240 would become a bit unwieldly trying to get all instances of foreach/Q_FOREACH fixed in one go, I've split the patches here along source/module boundaries. Hopefully this will make it easier to review, test and merge, as it can be merged before every instance is converted.

I'll keep pushing to this branch for now, but feel free to merge at any time once you're happy with it. I'll just make a new PR for the next batch of convertions.

For the most part, I've replaced foreach with range based for statements, but in some instances where I think it made more sense I've replaced with the std::find_ifalgorithm instead.

I think I've avoided any unneccessary detaching of Qcontainers, but let me know if you see anything suspicious.

er-vin · 2020-08-17T07:31:02Z

Please make sure to please the DCO bot.

er-vin · 2020-08-17T07:35:44Z

src/cmd/cmd.cpp

@@ -294,7 +294,7 @@ void selectiveSyncFixup(OCC::SyncJournalDb *journal, const QStringList &newList)
    if (ok) {
        auto blackListSet = newList.toSet();
        auto changes = (oldBlackListSet - blackListSet) + (blackListSet - oldBlackListSet);
-        foreach (const auto &it, changes) {
+        for (const auto &it : qAsConst(changes)) {


Note that the alternative would have been to declare blackListSet and changes as const auto. That'd work as well.

(I'm not asking you to change it if you don't want to, it's more of a "BTW" comment).

er-vin · 2020-08-17T07:36:52Z

src/cmd/simplesslerrorhandler.cpp

@@ -27,7 +27,7 @@ bool SimpleSslErrorHandler::handleErrors(QList<QSslError> errors, const QSslConf
        return false;
    }

-    foreach (QSslError error, errors) {
+    for (const auto & error : qAsConst(errors)) {


Style nitpick: there should be no space after &

er-vin · 2020-08-17T07:39:47Z

src/gui/accountmanager.cpp

-        foreach(QString key, settings.childKeys()) {
+        // copy container as it's being modified in for loop
+        const auto childKeys = settings.childKeys();
+        for (const QString & key : childKeys) {


No space after & please.

Optional: could have been const auto &key as well.

er-vin · 2020-08-17T07:41:25Z

src/gui/accountmanager.cpp

-        }
-    }
-    return AccountStatePtr();
+    auto acc = std::find_if(_accounts.cbegin(), _accounts.cend(), [&name](const auto &acc) {


I'd change auto acc for const auto it since what's returned isn't an AccountStatePtr but an iterator to an AccountStatePtr.

Also I'd capture name by value here (just to show the intent that we don't write to it).

er-vin · 2020-08-17T07:44:28Z

src/gui/accountmanager.cpp

-        }
-    }
-    return true;
+    auto acc = std::find_if(_accounts.cbegin(), _accounts.cend(), [&id](const auto &acc) {


Same comments as the find_if above.

Also probably better to just reuse AccountManager::account() at that point, no need to duplicate the code.

Looks like that one got forgotten

er-vin · 2020-08-17T07:46:51Z

src/gui/accountstate.cpp

-            if(app->id() == appId)
-                return app;
+        const auto apps = appList();
+        auto app = std::find_if(apps.cbegin(), apps.cend(), [&appId](const auto app) {


Same comments than before regarding the find_if use: auto app -> const auto it and capture appId by value.

Also in that case the app parameter to the lambda should be a const auto reference.

er-vin · 2020-08-17T07:58:43Z

src/gui/accountsettings.cpp

        if (folder->accountState() != _accountState) {
            continue;
        }

        bool ok = false;
        auto undecidedList = folder->journalDb()->getSelectiveSyncList(SyncJournalDb::SelectiveSyncUndecidedList, &ok);
        QString p;
-        foreach (const auto &it, undecidedList) {
+        for (const auto &it : qAsConst(undecidedList)) {


I think undecidedList itself could have been declared as const.

er-vin · 2020-08-17T07:59:25Z

src/gui/application.cpp

@@ -241,7 +241,7 @@ Application::Application(int &argc, char **argv)
        this, &Application::slotAccountStateAdded);
    connect(AccountManager::instance(), &AccountManager::accountRemoved,
        this, &Application::slotAccountStateRemoved);
-    foreach (auto ai, AccountManager::instance()->accounts()) {
+    for (const auto & ai : AccountManager::instance()->accounts()) {


No space after & please.

snake66 · 2020-08-17T17:32:19Z

I've fixed the issues you mention, rebased and pushed again.

One notable change in the latest version is that I have made lvalue refs where the previous patches used temporary rvalue refs returned from functions directly. After consulting the Stroustrup book on rvalue references, this seems safer.

Also got rid of some other deprecation warnings while going through this again. Still ample opportunities for refactoring, but think it's better to leave that for later iterations.

er-vin

Although that's legal, I'm not a huge fan of the const lvalue reference bound to temporaries. Just makes things more obscure, no safer and not more performant (since you got RVO anyway

er-vin · 2020-08-17T19:33:20Z

src/cmd/cmd.cpp

@@ -290,11 +290,12 @@ void selectiveSyncFixup(OCC::SyncJournalDb *journal, const QStringList &newList)

    bool ok = false;

-    auto oldBlackListSet = journal->getSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList, &ok).toSet();
+    const auto &bl = journal->getSelectiveSyncList(SyncJournalDb::SelectiveSyncBlackList, &ok);


Well, OK... that's allowed and the temporary lifetime will be prolonged. That said I'd avoid using that construct it's too obscure for most readers and likely to deduce the wrong thing. The gain from it is really marginal anyway.

(that applies to the whole patch)

In this particular case, the call to getSelectiveSyncList has to be invoked before the if statement on the next line because it potentially changes the conditional the if statement checks.

Since the QList::toSet member function is deprecated, I thought I'd move to the recommended way of constructing the set while I was at it. I can drop that part of the change if you prefer.

I have dropped the extra temporaries from the rest of the patch.

I'm slightly confused by your answer here. I wasn't talking about moving the getSelectiveSyncList call but about having bl as const value and not const reference.

Or did I misunderstand what you meant here?

Ah, no probably me who misunderstood here. I agree. I'll make it a value.

er-vin · 2020-08-17T19:37:14Z

src/gui/accountmanager.cpp

-        }
-    }
-    return true;
+    auto acc = std::find_if(_accounts.cbegin(), _accounts.cend(), [&id](const auto &acc) {


Looks like that one got forgotten

er-vin · 2020-08-17T19:41:47Z

One notable change in the latest version is that I have made lvalue refs where the previous patches used temporary rvalue refs returned from functions directly. After consulting the Stroustrup book on rvalue references, this seems safer.

Well, I seem to remember you received them on an lvalue so that was fine IMO. If you received them on rvalue references yes it'd have been an issue. :-)

er-vin · 2020-09-28T10:18:34Z

Ah you'll have to please the CI. Looks like you went for the deprecation warning for QSet in Qt 5.14 and up, but we depend on Qt 5.12 which doesn't yet have the replacement ctor.

Also please rebase on latest master.

snake66 · 2020-09-28T10:24:14Z

Ah you'll have to please the CI. Looks like you went for the deprecation warning for QSet in Qt 5.14 and up, but we depend on Qt 5.12 which doesn't yet have the replacement ctor.

Ah, sure. I'll drop the offending commit.

Also please rebase on latest master.

Will do.

er-vin · 2020-09-28T10:51:53Z

One notable change in the latest version is that I have made lvalue refs where the previous patches used temporary rvalue refs returned from functions directly. After consulting the Stroustrup book on rvalue references, this seems safer.

Well, I seem to remember you received them on an lvalue so that was fine IMO. If you received them on rvalue references yes it'd have been an issue. :-)

Also please go back to lvalue (const auto) for receiving the collections. My comment was really about the fact that going for const auto & although correct tends to be more obscure to other developers without any particular gain (neither in safety nor in performance might actually get worse longer term since it'll disable NRVO as far as I know).

snake66 · 2020-09-28T18:04:09Z

Wohoo, CI seems happy. @er-vin You happy too?

er-vin

Sorry to be so annoying. :-)

There are still a few "extended life time temporaries via const ref" left. I'd still advocate getting rid of them for the reasons I mentioned previously.

Also I realized (a bit late... should have spotted that earlier) that you're using std::find_if at places while really you meant std::any_of (since the iterator is never dereferenced just compared to cend()). So better use the more expressive of the two.

src/gui/accountsettings.cpp

src/gui/accountstate.cpp

er-vin · 2020-09-29T08:12:52Z

src/gui/folder.cpp

-        }
-    }
+    const auto &folderMap = FolderMan::instance()->map();
+    const auto it = std::find_if(folderMap.cbegin(), folderMap.cend(), [this](const auto *other) {


Could be std::any_of.

Went with std::none_of here too.

src/gui/folderman.cpp

er-vin · 2020-09-29T08:13:42Z

src/gui/folderman.cpp

-        }
-    }
+    const auto &folderList = FolderMan::instance()->map();
+    const auto it = std::find_if(folderList.cbegin(), folderList.cend(), [this, folder](const auto *other) {


Could be std::any_of.

...and here (std::none_of)

src/gui/folderman.cpp

er-vin · 2020-09-29T08:17:22Z

Also please rebase on latest master before the next push. Thanks in advance!

snake66 · 2020-09-29T08:40:29Z

Also please rebase on latest master before the next push. Thanks in advance!

I always rebase to latest master before pushing.

Signed-off-by: Harald Eilertsen <haraldei@anduin.net>

Most instances have been converted to range based for, but std::find_if has been used where it made sense. Signed-off-by: Harald Eilertsen <haraldei@anduin.net>

Signed-off-by: Harald Eilertsen <haraldei@anduin.net>

We're not interested in any found element in these cases, just to check that none of the elements matches. Signed-off-by: Harald Eilertsen <haraldei@anduin.net>

nextcloud-desktop-bot · 2020-09-29T17:37:30Z

AppImage file: Nextcloud-PR-2272-7721da25e553d6a65600d0fa6fab1bee32d7d308-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

er-vin

Good call for the std::none_of unsure why only std::any_of popped in my mind. :-D

snake66 mentioned this pull request Aug 15, 2020

2219 convert foreach #2240

Closed

er-vin reviewed Aug 17, 2020

View reviewed changes

er-vin requested changes Aug 17, 2020

View reviewed changes

snake66 force-pushed the issues/2219/deprecate-foreach branch from 60bb4e6 to 377fc26 Compare August 17, 2020 17:21

er-vin requested changes Aug 17, 2020

View reviewed changes

snake66 force-pushed the issues/2219/deprecate-foreach branch from 377fc26 to e0966f3 Compare September 27, 2020 12:26

snake66 force-pushed the issues/2219/deprecate-foreach branch from e0966f3 to dce5e66 Compare September 28, 2020 17:23

er-vin requested changes Sep 29, 2020

View reviewed changes

snake66 added 10 commits September 29, 2020 19:31

cmd: Replace foreach with range base for (nextcloud#2219)

5f4cec8

Signed-off-by: Harald Eilertsen <haraldei@anduin.net>

gui/accountmanager: Replace foreach/Q_FOREACH (nextcloud#2219)

1898782

Signed-off-by: Harald Eilertsen <haraldei@anduin.net>

gui/accountstate: Replace foreach (nextcloud#2219)

225fdab

Most instances have been converted to range based for, but std::find_if has been used where it made sense. Signed-off-by: Harald Eilertsen <haraldei@anduin.net>

gui/accountstate: Replace foreach (nextcloud#2219)

e343b4b

Signed-off-by: Harald Eilertsen <haraldei@anduin.net>

gui/application: replace foreach (nextcloud#2219)

5bd2529

Signed-off-by: Harald Eilertsen <haraldei@anduin.net>

gui/folder: Replace foreach (nextcloud#2219)

677997b

Signed-off-by: Harald Eilertsen <haraldei@anduin.net>

gui/folderman: Replace foreach (nextcloud#2219)

62bf4c5

Signed-off-by: Harald Eilertsen <haraldei@anduin.net>

gui/accountmanager: Make isAccountIdAvailable a bit clearer.

074a94c

Signed-off-by: Harald Eilertsen <haraldei@anduin.net>

Don't hold ref to value returned from function.

061ef02

Signed-off-by: Harald Eilertsen <haraldei@anduin.net>

Replace find_if with none_of where appropriate.

7721da2

We're not interested in any found element in these cases, just to check that none of the elements matches. Signed-off-by: Harald Eilertsen <haraldei@anduin.net>

snake66 force-pushed the issues/2219/deprecate-foreach branch from dce5e66 to 7721da2 Compare September 29, 2020 17:33

er-vin approved these changes Sep 29, 2020

View reviewed changes

er-vin merged commit 6569ecb into nextcloud:master Sep 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issues/2219/deprecate foreach #2272

Issues/2219/deprecate foreach #2272

snake66 commented Aug 15, 2020

er-vin commented Aug 17, 2020

er-vin Aug 17, 2020

er-vin Aug 17, 2020

er-vin Aug 17, 2020

er-vin Aug 17, 2020

er-vin Aug 17, 2020

er-vin Aug 17, 2020

er-vin Aug 17, 2020

er-vin Aug 17, 2020

er-vin Aug 17, 2020

snake66 commented Aug 17, 2020

er-vin left a comment

er-vin Aug 17, 2020

snake66 Sep 27, 2020

er-vin Sep 28, 2020

snake66 Sep 28, 2020

er-vin Aug 17, 2020

er-vin commented Aug 17, 2020

er-vin commented Sep 28, 2020

snake66 commented Sep 28, 2020

er-vin commented Sep 28, 2020

snake66 commented Sep 28, 2020

er-vin left a comment

er-vin Sep 29, 2020

snake66 Sep 29, 2020

er-vin Sep 29, 2020

snake66 Sep 29, 2020

er-vin commented Sep 29, 2020

snake66 commented Sep 29, 2020

nextcloud-desktop-bot commented Sep 29, 2020

er-vin left a comment

Issues/2219/deprecate foreach #2272

Issues/2219/deprecate foreach #2272

Conversation

snake66 commented Aug 15, 2020

er-vin commented Aug 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snake66 commented Aug 17, 2020

er-vin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

er-vin commented Aug 17, 2020

er-vin commented Sep 28, 2020

snake66 commented Sep 28, 2020

er-vin commented Sep 28, 2020

snake66 commented Sep 28, 2020

er-vin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

er-vin commented Sep 29, 2020

snake66 commented Sep 29, 2020

nextcloud-desktop-bot commented Sep 29, 2020

er-vin left a comment

Choose a reason for hiding this comment