[PATCH] D72282: [clang-tidy] Add `bugprone-unintended-adl`

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:89
+
+    const auto *Lookup =
+        Result.Nodes.getNodeAs<UnresolvedLookupExpr>("templateADLexpr");
----------------
logan-5 wrote:
> JonasToth wrote:
> > Can't you just bind directly to the `unresolvedExpr`?
> I need the `"templateADLCall"` node for the diagnostic caret, and the `"templateADLexpr"` for the name/spelling of the call. I might totally be misunderstanding what you're suggesting here.
Ah, i overlooked that bit. Then The `Lookup` should be `assert`ed as well, to ensure the matchers works in all circumstances. (future refactorings included)


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp:1
+// RUN: %check_clang_tidy -std=c++14-or-later %s bugprone-unintended-adl %t
+
----------------
logan-5 wrote:
> JonasToth wrote:
> > why 14 or later? `ADL` exists in the prior standards, too.
> > 
> > Additionally we need a test for where overloaded operators are not ignored and create the warnings.
> 14 or later just because of the generic lambdas in some of the tests. Is it worth separating those tests out into their own files so that we don't have to pass this flag here?
Yes. Usually we have the "base"-version of c++ for most of the test, that are common and extra test-files for newer features or requirements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72282