Quuxplusone added a comment.
Tangential data point: I hacked up this check to find //all// unqualified `std`
functions and ran it on libc++'s tests and also on some of my own codebase
(including re2, asio, protobuf). I'm fixing the issues it turned up in libc++'s
tests. In the other codebases, there was a lot of `min` and `max` in multiple
codebases; several uses of `back_inserter` and `getline` across different
codebases; scattered probably-accidental use of algorithms in protobuf (`sort`,
`stable_sort`, `binary_search`); one file contained `using std::setw` etc.; and
asio makes very heavy use of unqualified `declval`.
This is irrelevant to Corentin's interests with this PR; I'm just dumping the
finding here in case anyone's ever interested in generalizing this check and
wants to know what it would find.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:6437
+ NamedDecl *D = dyn_cast_or_null<NamedDecl>(Call->getCalleeDecl());
+ if (!D || !D->isInStdNamespace())
+ return;
----------------
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.
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
