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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4812
+def warn_unqualified_call_to_std_function : Warning<
+  "unqualified call to %0">, InGroup<DiagGroup<"unqualified-std-call">>;
+
----------------
This should probably be named `-Wunqualified-std-move` resp. `-Wunqualified-std-forward`, so that we preserve our ability to add a more generalized `-Wunqualified-std-call` diagnostic (probably not enabled by default) in the future.

I'd actually like to go ahead and add the generalized diagnostic right now, since it would be about 10 extra lines of code, and at least //I// would use it as soon as it landed in trunk! But I'm prepared for that to be controversial and wouldn't necessarily pick that hill to die on. :)  (Ah, and I'm reminded that the generalized diagnostic would need a bigger list of hard-coded functions to ignore, such as `swap` and `begin` and `end` and so on.)


================
Comment at: clang/test/SemaCXX/unqualified-std-call.cpp:34
+  std::move(foo);
+  move(foo); // expected-warning{{unqualified call to std::move}}
+
----------------
I'd like to see a test where the template finds `move` via ADL, rather than being directly in the scope of a `using namespace std` directive. This is the most important case AIUI, because we can train people not to `using namespace` but we can't so easily train them not to accidentally forget a `std::`.
I'd like to see a couple of tests where the (function, resp. template) is itself located inside `namespace std`. (You mention you deliberately want to warn on this, and I think that's good, but I don't see any tests for it.)
I'd like to see a test where the `std::move` that is found by lookup is actually `std::__1::move` with an inline namespace, because that's what libc++ actually does in practice. (This should still warn.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119670