erichkeane 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;
----------------
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.
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
