[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:6437
+  NamedDecl *D = dyn_cast_or_null<NamedDecl>(Call->getCalleeDecl());
+  if (!D || !D->isInStdNamespace())
+    return;
----------------
erichkeane wrote:
> Quuxplusone wrote:
> > erichkeane wrote:
> > > Do we really want this?  I guess I would think doing:
> > > 
> > > 
> > > ```
> > > void MyFunc(auto whatever) {
> > >   auto X = move(whatever);
> > > ```
> > > 
> > > when I MEAN std::move, just for it to pick up a non-std::move the 1st time is likely the same problem?  Or should it get a separate warning?
> > That's a good point (IMHO). Perhaps instead of making this a specific case of "warn for unqualified call to things in `std` (namely `move` and `forward`)," we should make it a specific case of "warn for any unqualified use of //this identifier// (namely `move` and `forward`)." That's closer in spirit to Nico Josuttis's comment that `move` is almost like a keyword in modern C++, and therefore shouldn't be thrown around willy-nilly. Either you mean `std::move` (in which case qualify it), or you mean some other `my::move` (in which case qualify it), but using the bare word `move` is inappropriate in modern C++ no matter whether it //currently// finds something out of `std` or not.
> > I'm ambivalent between these two ways of looking at the issue. Maybe someone can think up a reason to prefer one or the other?
> > 
> > libc++'s tests do include several recently-added instances of `move` as a //variable name//, e.g. `auto copy(x); auto move(std::move(x));`. This confuses grep but would not confuse Clang, for better and worse. I don't expect that real code would ever do this, either.
> > 
> > @erichkeane's specific example is a //template//, which means it's going to be picked up by D72282 `clang-tidy bugprone-unintended-adl` also. Using ADL inside templates triggers multiple red flags simultaneously. Whereas this D119670 is the only thing that's going to catch unqualified `move` in //non-template// code.
> > That's a good point (IMHO). Perhaps instead of making this a specific case of "warn for unqualified call to things in `std` (namely `move` and `forward`)," we should make it a specific case of "warn for any unqualified use of //this identifier// (namely `move` and `forward`)." That's closer in spirit to Nico Josuttis's comment that `move` is almost like a keyword in modern C++, and therefore shouldn't be thrown around willy-nilly. Either you mean `std::move` (in which case qualify it), or you mean some other `my::move` (in which case qualify it), but using the bare word `move` is inappropriate in modern C++ no matter whether it //currently// finds something out of `std` or not.
> 
> Ah! I guess that was just my interpretation of how this patch worked: Point out troublesome 'keyword-like-library-functions' used unqualified.  I think the alternative (warn for unqualified call to things in std) is so incredibly noisy as to be worthless, particularly in light of 'using' statements.
> 
> > @erichkeane's specific example is a //template//, which means it's going to be picked up by D72282 `clang-tidy bugprone-unintended-adl` also. Using ADL inside templates triggers multiple red flags simultaneously. Whereas this D119670 is the only thing that's going to catch unqualified `move` in //non-template// code.
> 
> This was a template for convenience sake (so y'all couldn't "well actually" me on the type I chose), but good to know we have a warning like that!
> 
> What I was TRYING to point out a case where the person is using `move` or `forward` intending to have the `std` version, but accidentially ending up with thier own version.
It is *likely* the same problem. The problem is the "likely" does a lot of heavy lifting here.
I think there is general agreement that std::move should be called qualified, not that `move` is somehow a special identifier that users should not use. These are almost contradictory statements - aka if we wanted to discourage people to name their function move, we wouldn't also need or want to force them to qualify their call.

It is very possible that `std::move` cannot be used at all in the TU because it is simply not declared, in which case the code would be perfectly fine.
A slightly more involved approach would be to try to detect whether std::move is declared by doing a lookup and warn in that case, but I'm not sure that brings much.
I certainly disagree that calling a function `move` is cause for warning.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119670/new/

https://reviews.llvm.org/D119670