src/plugin.cpp
196–199	Do we have rules in KF whether stl-like names liks cbegin() & cend() are fine? Then plugins is const, so begin()/end() are giving use the const iterators already. Does it make sense to catch the reference `library` by reference? Myself do not have a stable best-practice rules here, so happy to learn more.
src/statusbarextension.cpp
139–140	container elements are modified.
143–144	Same here :)

dfaure added inline comments.Sep 28 2019, 1:20 PM

src/plugin.cpp
196–199	STL is fine by definition, it's the C++ standard. But yes, no need for cbegin/cend on a const container. Catching `library` by reference makes sense, just like you wouldn't pass it by value to a function, here you're capturing it by value. The fact that it's a reference in the argument list doesn't change anything. See e.g. http://www.cplusplus.com/forum/general/142165/

kossebau added inline comments.Sep 28 2019, 1:49 PM

src/plugin.cpp
196–199	Meh, I need to check quite some code of mine then, I got that wrong and thought that the values of the actual variables listed are captured (i.e. for a reference type the reference "pointer"), and not that some kind of interpretation happens here. I unsure I understood the link (and other search hits I tried so far) and wonder, what actually is the type of `library` then in the lambda? A const reference, to a copy of the originally referenced object?

properly mark captured variable to be captured by reference as intended

Harbormaster completed remote builds in B17126: Diff 66996.Sep 28 2019, 2:21 PM

kossebau added inline comments.Sep 28 2019, 2:27 PM

src/plugin.cpp
196–199	Confirmed by experiments. Still not yet found a document where explicitly it is mentioned that the copy constructor will be invoked to generate a copy of the object for any captured variables only being of type reference, so if anyone can point out one which reads this clearly to me, happy to get a reference to, so I can try to do a copy of that referenced document into my brain lambda :)

It's actually quite clear in my head, because I imagine the generated class. A captured variable in a lambda becomes a member variable. If it's a capture by value (which is what happens with [library]), it's a "plain value" member.
So:

bool Plugin::hasPlugin(const QString &library)
{
    auto func = [&library](QObject* p) {
        return ...;
    });
}

becomes

bool Plugin::hasPlugin(const QString &library)
{
    class Closure {
        bool operator(QObject *p) const { return ...; }
        QString library;
    };
    auto func = Closure{library};
}

or something like that.

dhaumann added inline comments.Sep 28 2019, 7:55 PM

src/plugin.cpp
196–199	Understanding captures becomes simple as soon as you follow what the compiler essentially does for you with the lambda: The lambda function does not capture `[this]`, so behind the scenes the compiler will create for you a free function (with an internal unique function name). This free function will have a local const variable "library". If you write [library] than this local variable will simply be a copy (think of the definition `const QString library = library_from_outer_scope;`. When you write `[&library]` instead, you will get a `const QString &library = library_from_outer_scope;`. Does that clarify when to use `&` or not? :-) PS: If you capture `[this]`, then the compiler will not a create a free function, instead the compiler will create a member function for you (again with internal unique name ....). Looking at lambda functions from this perspective makes this rather simple.

In D24262#539317, @dfaure wrote:

It's actually quite clear in my head, because I imagine the generated class. A captured variable in a lambda becomes a member variable. If it's a capture by value (which is what happens with [library]), it's a "plain value" member.
So

David's explanation is much more correct: Behind the scenes you have this Closure trick with a functor object, not really a free function what I was claiming ;)

Thanks for your teaching, appreciated :) Will have another look again when not tired. Just tried again to read on cppreference.com the stuff about lambda capturing, but still not digested what I read this afternoon, reread now did not help. So next try scheduled.

So far my thinking was, given some variables in the scope Type& ref; Type* pointer; Type type;,
it would be for [&]

class Closure {
    bool operator() const { return ...; }
    Type&& ref; // yes, no idea if this even is supported by C++, that was just my mind model
    Type*& pointer;
    Type& type;
};

and it would be for [=]

class Closure {
    bool operator() const { return ...; }
    Type& ref;
    Type* pointer;
    Type type;
};

So seems I have to rewrite my mind model for the type of ref.

@kossebau: did you try the C++ Standard (working draft): https://isocpp.org/blog/2013/10/n3797-working-draft-standard-for-programming-language-c-stefanus-du-toit

This ^ one is circa 2012.

All the sources used to generate the C++ Standard drafts are maintained in this git repo:
https://github.com/cplusplus/draft

(Reading the standard feels like reading legalese, but it's quite informative albeit very^Wslightly mind boggling).

Sorry if this is off-topic.

Edit: phabricator doesn't support removing inline comments? (stupid, stupid)

src/plugin.cpp
196–199	An entity is captured by copy if it is implicitly captured and the capture-default is = or if it is explicitly captured with a capture that does not include an &. For each entity captured by copy, an unnamed non- static data member is declared in the closure type. The declaration order of these members is unspecified. The type of such a data member is the type of the corresponding captured entity if the entity is not a reference to an object, or the referenced type otherwise. [ Note: If the captured entity is a reference to a function, the corresponding data member is also a reference to a function. — end note ] An entity is captured by reference if it is implicitly or explicitly captured but not captured by copy. It is unspecified whether additional unnamed non-static data members are declared in the closure type for entities captured by reference.

Please make the loop variable const-ref whenever possible.

src/browseropenorsavequestion.cpp
270–271	Why not const auto &app?
src/plugin.cpp
138–139	const auto & (space before '&', btw, not after)
214–215	const...
src/scriptableextension.cpp
260	const

This revision now requires changes to proceed.Sep 29 2019, 9:44 PM

kossebau added inline comments.Sep 29 2019, 9:53 PM

src/browseropenorsavequestion.cpp
270–271	In most projects I have seen people omitting the const, and only adding explicitly any &/* to help the human reader a bit more. If you prefer explicit const, fine with me, personally undecided how much auto is good :)

In D24262#539348, @ahmadsamir wrote:

@kossebau: did you try the C++ Standard (working draft): https://isocpp.org/blog/2013/10/n3797-working-draft-standard-for-programming-language-c-stefanus-du-toit

This ^ one is circa 2012.

All the sources used to generate the C++ Standard drafts are maintained in this git repo:
https://github.com/cplusplus/draft

(Reading the standard feels like reading legalese, but it's quite informative albeit very^Wslightly mind boggling).

Thanks. Yes, quite legalese, but it narrowed things down to some good degree for me now. The quote you made in the inline comment seems indeed to be the one I should grasp: "The type of such a data member is the type of the corresponding captured entity if the entity is not areference to an object, or the referenced type otherwise." (about "captured by copy"). I think I start to make this known to me :) [Edit; and while checking own commits from recent times already found code from others where the missing & results in value copies :) ]

dfaure added inline comments.Sep 29 2019, 10:14 PM

src/browseropenorsavequestion.cpp
270–271	auto or not auto isn't the question when it comes to const :-) Would you have written `foreach(QString &app, apps)` if `apps` was a qstringlist (to simplify)? Or `for (QString &app : apps)` ? You say "in most projects I have seen", but my reference is Qt, please find me one place in Qt where a range-for isn't using const when the item isn't modified inside the loop :) And while * is redundant (therefore it only helps humans), the & is definitely not redundant. Without it you get a copy.

move &/* to varname
explicit const with auto types

Harbormaster completed remote builds in B17159: Diff 67046.Sep 29 2019, 10:14 PM

kossebau added inline comments.Sep 29 2019, 10:33 PM

src/browseropenorsavequestion.cpp
270–271	Sigh... I learned C++ originally from the Stroustrup book, mostly reading in bed in leisure time. C++11 I learned from peers and the internet on the job, and now find myself often with misconceptions. I start to think I should return to proper books in bed :) And seem references are a thing I tend to be wrong on, eh.

dfaure accepted this revision.Sep 30 2019, 7:58 AM

dfaure added inline comments.

src/browseropenorsavequestion.cpp
270–271	I give C++11/14/17 trainings, if it helps :-)

This revision is now accepted and ready to land.Sep 30 2019, 7:58 AM

kossebau added inline comments.Sep 30 2019, 11:59 AM

src/browseropenorsavequestion.cpp
270–271	:) Though, I doubt you give them in single person understanding-pace-driven tiredness-cut bed settings (besides own family.... how to tell a nerd kid: "Mummy/Daddy, please, please, another good night tutorial!").

Closed by commit R306:65c55de00bae: Modernize code: use range-based loops & algorithms in more places (authored by kossebau). · Explain WhySep 30 2019, 1:26 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			src/browseropenorsavequestion.cpp (4 lines)
M			src/partbase.h (6 lines)
M			src/partmanager.cpp (10 lines)
M			src/plugin.cpp (40 lines)
M			src/scriptableextension.cpp (4 lines)
M			src/statusbarextension.cpp (10 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	66950	c33efaa		Sep 27 2019, 2:27 PM	★	★
Diff 2	66996	c33efaa	properly mark captured variable to be captured by reference as intended	Sep 28 2019, 2:21 PM	★	★
Diff 3	67046	c33efaa	* move &/* to varname	Sep 29 2019, 10:14 PM	★	★
Diff 4	67067	c33efaa	R306:65c55de00bae476ea34918df7fb6fe0ac289f3b1	Sep 30 2019, 11:59 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
04562792bbb5	12db2e945f71	c33efaae83bb	Friedrich W. H. Kossebau	Modernize code: use range-based loops & algorithms in more places (Show More…)	Sep 27 2019, 2:18 PM