Hi Florian, you're right. The example was made up on the spot, so not based on a
real missing "move". ;-)

Changed the title to reflect what we actually did here.

I merged the issue branch.

I opened issue856 to add (among two other checks) a clang-tidy check for the kind 
of use-after-move error you mention in msg7987 and msg7991.

Shouldn't this be "data(move(data))" instead of "data(data)" (the parameter
takes an rvalue reference but is has a name so it is an lvalue reference
itself)? I'm just asking because if you adapted this example from some real
code, we might have an accidental copy there.

Looks good to merge to me.

For what it's worth, I was suprised that constructors were excluded from
-Wshadow in clang because most issues with shadowing that I remember coming up
in our code happened in constructors, in code along the following lines:

FooBar::FooBar(vector<int> &&data, ...)
    : data(data) {
    /* Do some further processing with "data", for example
       computing some kind of index data structure or doing
       some initialization based on data.size(). */
    ...
}

This will not work because "data" inside the method body refers to the argument,
not the attribute, and this will be empty after the move that happens in the
"data(data)" initialization.

I updated the pull request according to my previous message (msg7954). Could 
someone do a quick review, please?

I think these are valid concerns and would probably already justify not adding 
-Wshadow. However, I just found out about a more important reason not to add 
the flag: GCC with -Wshadow warns about constructor arguments shadowing class 
members (see 
https://bitbucket.org/jendrikseipp/downward/addon/pipelines/home#!/results/483)
. We haven't seen this error before because I used Clang locally and the 
Bitbucket tests were not run because we were over our budget of 500 free build 
minutes per month. I think this settles the discussion. I'll cherry-pick the 
changes we like from the diff and revert the others.

I don't think "-Wshadow" will make working with the code more beginner-friendly
because it breaks correct C++ code that some people might consider idiomatic.
For example, people might legitimately want to write things like

void MyClass::set_variable(int variable) {
    this->variable = variable;
}

and might be surprised to see this break the build.

More generally, shadowing class variables with local variables is not
particularly uncommon (especially when the variables refer to the same kind of
thing), and it is something that people are used to being able to do. So
initially, I think this will make things less intuitive and somewhat confusing.
The advantage is in providing a safety net against a potential source of errors,
but I think the number of false positives will be much higher than the number of
true positives. (Case in point: the 20-30 changes we have to make to address the
false positives in the current codebase.) 

So if there's an overall benefit I think it must be that we consider the
potential benefit gained from true positives as much higher than the cost of the
false positives.

One thing that slightly concerns me is that the change can break correct user
code, and the breakage can happen "at a distance". If we rename some internal
attribute of a common base class like "Evaluator", and if a user's (currently
correctly working) evaluator happens to use the new name as a local variable
somewhere, then that code will suddenly no longer work. (That problem generally
exists in C++ when the conflict is between class members and subclass members,
but conflicts with local variables tend to be much more common.)

I have changed the example you mention to use long names for the attributes 
(variable, value) and short names for the names in the methods (var, val). 
Also, I merged the latest master into the issue branch. I am leaning towards 
adding the -Wshadow flag because I think the new names aren't too bad now and 
many of them make the code easier to understand. Especially for beginners, the 
new flag could make working with the code easier, I think.

I find change like the one from

    helper->var = var;
    helper->value = value;

to

    helper->var = variable;
    helper->value = val;

really grating. The old code is very clear. In the new code we have to
simultaneously replace "var" with something longer and "value" with something
shorter to avoid name clashes, so not only do we use different words for the
same thing, we also move two names that work together into opposite directions.

So I'd be against merging the code as-is. But it's certainly possible to solve
the naming problem one way or the other. The main question is whether the
utility of the -Wshadow flag is actually positive given that it often forces us
to use non-obvious names just to avoid the warning.

Anyone with a strong opinion for or against?

It will warn if a local shadows a local. (I just tested this.)

I don't think I had ever problems with shadowing an instance variable by a local
one; but I do remember that I had problems with shadowing a local variable by
another one, e.g., in nested loops :-) but I assume this warning would not help
in such a case, would it?

At the same time, I'm not so much attached to the existing variable names that
may be better than some of the ones Jendrik changed. So in summary, I'm somewhat
indifferent towards adding this feature or not.

Having looked at the pull request in detail, we should definitely use
-Wnon-virtual-dtor, but I'm skeptical about -Wshadow. I like the fact that it
can potentially catch issues, but I'm not sure they are the kind of issues that
would be hard to spot, and there clearly seems to be a cost in the sense that we
seem to be replacing many names with poorer names because the good names are
"taken".

At the moment, I'm slightly inclined not to add -Wshadow, which means we could
revert all the code changes. (Of course, we can still keep the code changes we
like if we want.) What do the others think? (You might want to look at the pull
request and its comments.)

Done handling your comments. Naming things is hard :-)

Done with my comments.

I propose to only add -Wnon-virtual-dtor and -Wshadow in this issue. The former 
actually requires no changes to the code. The latter requires some changes, but 
constructor parameter name clashes are ok.

Here is a pull request:
https://bitbucket.org/jendrikseipp/downward/pull-requests/104

I think we should go through the output of -Weffc++ in a different issue.

I'd be interested in adding more warnings, but I'm not sure how soon I could
find time to look into the options behind your gitbooks link more thoroughly.

I often look at "man gcc" for various reasons, and at first glance the gcc link
you gave is identical to the man page. Just a hint for those who don't often use
man pages. :-) (One advantage of man is that you know it matches your compiler
version, unless you're working on a badly misconfigured system.)

Have you tested -Wshadow? With our convention for constructor parameters, won't
this warn on every constructor parameter?

Even if -Weffc++ is too noisy, I think it would be useful to go through its
complete output once and see if there are true positives and if there are
further options we should enable. (But from the docs, it doesn't look like it's
possible to pick and choose there.)

Continuing the discussion from issue848, I think using -Weffc++ might be a bit too drastic since it has a 
lot of "false positives", but we could think about adding individual warnings (which will then get 
converted to errors).

I propose to add -Wnon-virtual-dtor to catch the errors we saw in issue848 and -Wshadow (found on  
https://lefticus.gitbooks.io/cpp-best-practices/content/02-Use_the_Tools_Available.html under GCC/Clang). 
Here is the full list of options: https://gcc.gnu.org/onlinedocs/gcc/Option-Summary.html

Using -Wnon-virtual-dtor shows no additional problems, but -Wshadow just uncovered a lurking bug in 
PatternCollectionGeneratorGenetic where an AbstractTask is shadowed. There might be more shadowing going 
on, but I just looked at the first error.

What do you think about adding those two warnings? Do you have other warnings that we should add?