Quuxplusone 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;
----------------
cor3ntin wrote:
> 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.
> I certainly disagree that calling a function `move` is cause for warning.
Right, agreed. For example, `boost::move` exists, and that's fine. (I wonder if
any codebase contains the line `using boost::move;`!)
> 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.
Well, the original raison d'etre for D119670 was the observation that `using
namespace std::views; auto y = move(x);` will change behavior in C++23, if
C++23 adds `std::views::move` as a view adaptor. There's nothing in that story
that involves a //user-defined// function named `move`; the reason we want to
warn people off of unqualified `move` (or unqualified //anything//-in-std) is
for SD-8-adjacent reasons: we want the library to be free to add new entities
(such as `std::views::move` specifically) without breaking old code. (Note: the
above code involves a second red flag — `using namespace` — so we don't expect
it to be //common//.)
> 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.
Strong disagree. Maybe `std::move` isn't visible //today//, but then tomorrow
you `#include <utility>` or some random header that transitively includes the
relevant piece of `<utility>`, and boom, //now// your `auto y = move(x)` does
something different. Unqualified single-arg `move` is a warning-worthy time
bomb no matter what, IMO. ...Which I guess means I'm coming around to favor
Erich's suggestion (at least so far this morning :))
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119670/new/
https://reviews.llvm.org/D119670
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
