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
