EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.
- This check should suggest fixes. It knows what function is actually being
resolved, and it has enough information to create a minimally qualified name
that resolves it.
- This check should ignore hidden friends, since their entire purpose is to be
called via ADL.
- This check should allow whitelisting namespaces that opt-out of ADL into
their namespace. This makes it much easier to roll out the clang-tidy
incrementally.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h:27
+class UnintendedADLCheck : public ClangTidyCheck {
+ const bool IgnoreOverloadedOperators;
+ const std::vector<std::string> AllowedIdentifiers;
----------------
I think we should always ignore operators. I don't see value in having a mode
where every comparison triggers this warning.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp:54
+ move(a);
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ns::move'
through ADL [bugprone-unintended-adl]
+ forward(a);
----------------
JonasToth wrote:
> logan-5 wrote:
> > Quuxplusone wrote:
> > > This is awesome. :) Please also consider ADL where there's more than one
> > > associated namespace, something like this: https://godbolt.org/z/S73Gzy
> > > ```
> > > template<class T> struct Templated { };
> > > Templated<std::byte> testX() {
> > > Templated<std::byte> x;
> > > using std::move;
> > > return move(x); // "correctly" resolves to std::move today, but
> > > still does unintended ADL
> > > }
> > > ```
> > > Please add a test isomorphic to the above, unless you think it's already
> > > covered by one of the existing tests.
> > It's interesting, that code only triggers the check (i.e. my AST matchers
> > only think it's doing ADL) without the `using std::move`. I admit I'm a bit
> > confused as to why.
> The matcher itself only ask the `CallExpr->usesADL()` (not sure about the
> method name right now, but basically that). So the reason is probably hidden
> somewhere in `Sema`, if the matcher is the issue.
A call expression only "usesADL" if the callee was found only through ADL
lookup. If it's found through normal lookup, then it doesn't "use adl", even
though ADL lookup is also performed and that ADL lookup considers `std::move`.
I've been considering adding `CallExpr::considersADL`, but I'm not convinced
it's needed.
There's more than enough work to be done cleaning up call's that depend on ADL.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp:202
+void macro_test(T t) {
+#define MACRO(x) find_if(x, x, x)
+
----------------
Arguably this is *exactly* the kind of code we want to diagnose.
The call in the macro either,
* Is a "customization point" and should be whitelisted. Or,
* It resolves the same in expansion (and can be qualified), Or,
* It is a bug.
You mentioned false positives in things like `assert`. Can you provide examples?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72282/new/
https://reviews.llvm.org/D72282
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
