C++ PATCH for c++/87029, Implement -Wredundant-move

[Marek Polacek]

On Thu, Aug 23, 2018 at 09:51:43AM -0400, David Malcolm wrote:
> On Wed, 2018-08-22 at 20:12 -0400, Marek Polacek wrote:
> > Now that we have -Wpessimizing-move implemented, it was fairly easy
> > to extend
> > it to -Wredundant-move, which warns about redundant calls to
> > std::move; it's
> > supposed to detect cases where the compiler is obliged to treat an
> > object as
> > if it were an rvalue, so calling std::move is pointless.  Since we
> > implement
> > Core Issue 1579, it warns in more situations than clang++.  I
> > described this
> > in more detail in the manual entry.
> > 
> > David, I've also tweaked the warning to use fix-it hints; the point
> > is to turn
> > 
> >   return std::move (x);
> > 
> > into
> > 
> >   return x;
> > 
> > I didn't fool around with looking for the locations of the parens;
> > I've used a
> > DECL_NAME hack instead.  I've verified it using -fdiagnostics-
> > generate-patch,
> > which produces e.g.:
> > 
> > @@ -27,7 +27,7 @@
> >  f ()
> >  {
> >    T foo;
> > -  return std::move (foo);
> > +  return foo;
> >  }
> >  
> > g++.dg/diagnostic/move1.C tests the fix-it hints for -Wredundant-move 
> > and
> > -Wpessimizing-move.
> 
> Thanks (and ideally, we'd have a precanned way for DejaGnu to test that
> the fix-its hints work, and lead to code that resolves the diagnostics;
> I've toyed around with doing so, but it's fiddly, involving a second
> compile; ugh).
> 
> Regarding the fix-it hint code: I'm not convinced that it works in
> every possible scenario.
> 
> Consider:
> 
> (A):
> 
>   namespace ns {
>      int some_global;
>   };
> 
>   int fn ()
>   {
>     return std::move (ns::some_global);
>   }
> 
> which I believe will generate this bogus suggestion:
> 
>     return std::move (ns::some_global);
>            ~~~~~~~~~~^~~~~~~~~~~~~~~~~
>            some_global
> 
> since IIRC, DECL_NAME doesn't contain any explicit namespace for the
> way the var was referred to.
> 
> (B):
> 
> #ifdef SOME_CONFIG_FLAG
> # define CONFIGURY_GLOBAL global_a
> #else
> # define CONFIGURY_GLOBAL global_b
> #endif
> 
>   int fn ()
>   {
>      return std::move (CONFIGURY_GLOBAL);
>   }
> 
> which presumably will erroneously strip out the macro in the fix-it
> hint:
> 
>      return std::move (CONFIGURY_GLOBAL);
>             ~~~~~~~~~~^~~~~~~~~~~~~~~~~~
>             global_a
> 
> rich_location will discard fix-it hints for locations that are in macro
> expansions to try to avoid problems like this, but (B)'s location will 
> be a non-macro location, I believe.

You're right; I realized that it would not handle e.g. parenthesized
expressions correctly, either.

> Hence my suggestion to, if possible, use the locations of the parens,
> so that the fix-it hint preserves the user's spelling of the variable.
>  
> But I appreciate that it might be unreasonably difficult to get at
> those locations.  Hence, I think the fix-it hint part of the patch is
> probably good enough as-is: I'd rather not let "perfect be the enemy of
> the good" here.

Indeed, my attempts at extracting the proper locations failed.  I went back
to good ol' inform for now as I no longer think that the DECL_NAME hack is
good enough.

Thanks for the help!

Marek