The clang-tidy check was ignoring all errors. I just fixed this with this commit: 
https://github.com/aibasel/downward/commit/aefe96a2bffce6a9e9433981acc10dfc06357411

I'm not sure whether this earlier wrong behaviour was the source of the confusion below.

Now, when I enable the performance-move-const-arg check without any additional parameters, I get

/home/jendrik/projects/Downward/downward/src/search/pdbs/pattern_collection_generator_multiple.cc:87:35: warning: std::move of the const variable 'pattern' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg]
/home/jendrik/projects/Downward/downward/src/search/pdbs/pattern_collection_generator_disjoint_cegar.cc:41:9: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg]
/home/jendrik/projects/Downward/downward/src/search/pdbs/cegar.cc:40:15: warning: std::move of the const variable 'pdb' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg]
/home/jendrik/projects/Downward/downward/src/search/pdbs/cegar.cc:41:16: warning: std::move of the const variable 'plan' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg]
/home/jendrik/projects/Downward/downward/src/search/merge_and_shrink/factored_transition_system.cc:68:30: warning: std::move of the const expression of the trivially-copyable type 'const bool' has no effect; remove std::move() [performance-move-const-arg]
/home/jendrik/projects/Downward/downward/src/search/merge_and_shrink/factored_transition_system.cc:69:30: warning: std::move of the const expression of the trivially-copyable type 'const bool' has no effect; remove std::move() [performance-move-const-arg]
/home/jendrik/projects/Downward/downward/src/search/merge_and_shrink/factored_transition_system.cc:70:26: warning: std::move of the expression of the trivially-copyable type 'int' has no effect; remove std::move() [performance-move-const-arg]
/home/jendrik/projects/Downward/downward/src/search/merge_and_shrink/transition_system.cc:137:14: warning: std::move of the const variable 'labels' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg]

If you run run-clang-tidy.py, it outputs the command it is running. You can just copy it and run it stand-alone.

To use the option of the check, you can use: -config="{CheckOptions: [{key: performance-move-const-arg.CheckTriviallyCopyableMove, value: 
0}]}"

What do I need to do to see the warnings? (I saw we use a "quiet" option, but I interpreted the docs as only suppressing *statistics* about warnings, but not the warnings themselves. Am I misreading the docs?)

The messages I get are only warnings and ignored by our script. But if we want to ignore warnings and the check only 
always emits warnings, not errors, then there is no point in running the check.

We get hundreds of warnings generated, but only very few show up with an additional message. With all checks enabled, we 
get 9 warnings with a message, 7 from performance-move-const-arg (resp. 6 with the option) and 2 from readability-
redundant-string-init (where we unnecessarily explicitly initialize a string as ""). But maybe this is just because 
these two checks are less quiet in quiet mode.

When looking into this, I also noticed that clang-tidy does not complain if we specify a check that does not exist. 
Besides misc-move-const-arg, also "misc-move-constructor-init", "misc-use-after-move" and "performance-implicit-cast-in-
loop" must still be replaced to have effect with clang-tidy-12. So we in any case need to touch the clang-tidy config 
again.

I looked into this and also got a clean bill from our tests after enabling this new check. Gabi, can you reproduce what you got? Did you use clang-tidy-12?

If not, I suggest we add the check (option 3).

I uncommented the additional check and tried to reproduce Gabi's output, but failed. Both with the old and new name of the check. For me, clang-tidy-12 happily accepts the current version of the code. If somebody else wants to pursue this: the test script should print the command for fixing a subset of the reported problems automatically. Not sure if there are automatic fixers for performance-move-const-arg, though.

Then do you want to prepare a pull request that makes this change (enables the check and makes the necessary code changes to get rid of the things it complains about)? I think we would need to look at the suggested changes before committing to adding this.

I have no strong opinion on this, but I'm still leaning towards option 2 or 3, because the examples show that move semantics are hard and the check might help newcomers getting them right.

> Malte suggested to use it with option CheckTriviallyCopyableMove=0
> but I am not sure whether this is exactly what we want.

That was really a compromise suggestion to give Jendrik some of the checks he wanted to add. I don't have a strong opinion on this, at least not without having (re-) checked the impact that these warnings would have on the code.

But I would like to close this issue, either by adding more checks or deciding not to add more checks. Currently, style/run-clang-tidy.py says:

    checks = [
        # Enable with CheckTriviallyCopyableMove=0 when we require
        # clang-tidy >= 6.0 (see issue856).
        # "misc-move-const-arg",

I think we have three main options, now that we're long past version 6.0:

1) Just remove the comment, don't add checks.
2) Remove the comment, add the check with CheckTriviallyCopyableMove=0.
3) Remove the comment, add the check without CheckTriviallyCopyableMove=0.

I'd be OK with all of them in principle, but for 2) and 3) I'd like to again look at the diffs they produce to see whether we like them or not.


The 2-3 (depending on whether we use CheckTriviallyCopyableMove=0 or not) warnings that Gabi quoted are just examples, right? Not the full set of things we'd have to change to get rid of the warnings?


I think none of these two or three warnings are currently very useful, but if the warning option might uncover other useful things in the future, I'd be OK with changing the code to get rid of the move calls. We've stumbled over some move-related performance issues in the past, and perhaps this warning would have caught some.

There is also the consideration that we might no longer need the method that produces these three warnings at all: are we still on a Visual C++ version that doesn't support using "default" here, which is the only reason why this function definition exists? If not, we should get rid of the manual definition and instead define it as default.


But if there would be more necessary changes, we'd need to check how much we like/dislike these.


@Jendrik: still in favour of adding the option? Based only on these examples, I'd also be OK with the "full" solution of option 3) above. I don't think the warning against trivially copyable moves adds real value, but also adding the CheckTriviallyCopyableMove=0 flag would be a complication that it would be nice to avoid.Anyone else got


@Everyone else: got an opinion on this?

Since we are now moving to clang-tidy-12, I would like to reopen the discussion because there was still the issue with misc-move-const-arg.

It seems that the check is now called performance-move-const-arg (https://releases.llvm.org/12.0.0/tools/clang/tools/extra/docs/clang-
tidy/checks/performance-move-const-arg.html).

Malte suggested to use it with option CheckTriviallyCopyableMove=0 but I am not sure whether this is exactly what we want.

Here is an example:

Without the option, I get the following three warnings for factored_transition_system.cc:

clang-tidy-12 --use-color -header-filter=.*,-tree.hh,-tree_util.hh -checks=-*,performance-move-const-arg -p=/home/roeger/repos/downward/downward-
roeger/builds/clang-tidy -quiet /home/roeger/repos/downward/downward-roeger/src/search/merge_and_shrink/factored_transition_system.cc
3 warnings generated.
68:30: warning: std::move of the const expression of the trivially-copyable type 'const bool' has no effect; remove std::move() [performance-
move-const-arg]
      compute_init_distances(move(other.compute_init_distances)),
                             ^                            ~
69:30: warning: std::move of the const expression of the trivially-copyable type 'const bool' has no effect; remove std::move() [performance-
move-const-arg]
      compute_goal_distances(move(other.compute_goal_distances)),
                             ^                            ~
70:26: warning: std::move of the expression of the trivially-copyable type 'int' has no effect; remove std::move() [performance-move-const-arg]
      num_active_entries(move(other.num_active_entries)) {


With the option set to false, the two warning for moving *const* expressions still persist:

clang-tidy-12 --use-color -header-filter=.*,-tree.hh,-tree_util.hh -checks=-*,performance-move-const-arg --config="{CheckOptions: [{key: 
performance-move-const-arg.CheckTriviallyCopyableMove, value: 0}]}" -p=/home/roeger/repos/downward/downward-roeger/builds/clang-tidy -quiet 
/home/roeger/repos/downward/downward-roeger/src/search/merge_and_shrink/factored_transition_system.cc
2 warnings generated.
68:30: warning: std::move of the const expression of the trivially-copyable type 'const bool' has no effect; remove std::move() [performance-
move-const-arg]
      compute_init_distances(move(other.compute_init_distances)),
                             ^                            ~
69:30: warning: std::move of the const expression of the trivially-copyable type 'const bool' has no effect; remove std::move() [performance-
move-const-arg]
      compute_goal_distances(move(other.compute_goal_distances)),

Is this the intended behavior? Put differently, are these things we would like to change in the code? Or should we continue to skip this test?

Merged.

Looks good to merge.

I updated the pull request accordingly.

I think post-processing the output like this is adding too much machinery to
achieve something which is not currently a big need (or else it would have
caught something).

I'd be happier to not add that option now and add it with
CheckTriviallyCopyableMove=0 once we move to a sufficiently modern clang-tidy
version.

I agree that forbidding to move trivially-copyable types might be a little too restrictive. This is probably why 
the Clang developers introduced an option to switch off the check in clang-tidy 6.0: 
http://releases.llvm.org/6.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/performance-move-const-arg.html 
(Note that the check has been put into a different category in this version.)

The output for a failed check of this type is

[...]/src/search/cegar/abstract_state.cc:22:12: warning: std::move of the expression of the trivially-copyable 
type 'cegar::Node *' has no effect; remove std::move() [misc-move-const-arg]

Since we grep the output for warnings and errors anyway, we could ignore this sub-type of error.

Done looking at the pull request. As mentioned on the tracker, I don't think the
code changes suggested by clang-tidy are good.

They are three changes, but they are all effectively the same thing. In all
cases, we have a manually implemented default move constructor because old
Visual Studio does not support "= default" for move constructors. clang-tidy
suggests an optimization that replaces trivial element moves by trivial element
copies, some elements would be moved and others copied.

I expect both our code and the modified code will be identical after
compilation. The compiler should be smart enough to compile away the equivalent
of `x.element = nullptr;` for an rvalue reference x where x.element is not used
afterwards, which is what this boils down to on the low level. Our current code
is what `= default` is actually defined us and is thus more explicit. I think
changing it makes the reader wonder why we move some elements and copy others,
and the answer is "to manually do a trivial optimization that the compiler can
also do automatically", which is not convincing.

Of course, once we go to a new Visual Studio, all three methods would go away
anyway, in which case there would be no diff. In that case, the
"misc-move-const-arg" test (which is unfortunately named because it doesn't do
just that) would not produce a diff, so it would not harm with the current code,
but I'm also not sure it would be a benefit. It does three things at the same
time; if it only did one or two of these, that might work for us, but I'm not
sure "never move a trivially-copyable type" is a rule we want to enforce.

From what I've seen so far, the other two tests look good. And it looks like
they would not lead to code changes, which would mean our code is already good. :-)

Hi Jendrik, thanks for the pull request. Does clang-tidy only produce diffs, or
can it also show explanations of what it is doing and why, like compiler
warnings? Unfortunately I cannot easily test this on my side at the moment. I
would be curious what the diagnostics are, if there are any.

I made a pull request at
https://bitbucket.org/jendrikseipp/downward/pull-requests/106 .

The misc-move-const-arg check detects that we sometimes move trivially-copyable 
objects.

That previous sentence one verb too many.

Can you tell append what they would currently output?

I propose to add three checks relating to the use of std::move():

http://releases.llvm.org/5.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/misc-move-const-arg.html
http://releases.llvm.org/5.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/misc-move-constructor-init.html
http://releases.llvm.org/5.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/misc-use-after-move.html

These checks should help us spot subtle std::move() errors.