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

fx-carton added a comment. Great check! Would it be possible to add an option to show the warning if a `using` directive is used? As in: namespace ns { struct s {}; void func(const s&) { } } using ns::s; using ns::func; // without this the check is triggered in

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

logan-5 added a comment. It's been a year and a half since I heard anything about this patch and I'd honestly kind of forgotten about it. I'm still around/lurking though, and would assist with getting it landed if it's still good to go. Comment at: clang-tools-extra/clang-tid

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

FelixDombek removed a reviewer: EricWF. FelixDombek added a comment. This revision is now accepted and ready to land. @EricWF requested changes which have since been added, but he has been inactive for half a year; what is the process to go forward with this? CHANGES SINCE LAST ACTION https:/

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

logan-5 added a comment. Pinging this. I believe all feedback from @EricWF was addressed back in my update to the patch in March. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72282/new/ https://reviews.llvm.org/D72282 ___ cfe-commits mail

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

logan-5 updated this revision to Diff 250272. logan-5 marked 2 inline comments as done. logan-5 added a comment. Re-juggled and made `bool OnlyDependsOnFundamentalArraySizes(QualType QualTy)` recursive. Made `::` assert more useful. Thanks @Quuxplusone for both good ideas. CHANGES SINCE LAST A

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

Quuxplusone added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:51 + // Remove the '::' at the end. + assert(NNS.size() >= 2); + NNS.erase(NNS.end() - 2, NNS.end()); As long as you're asserting anyway, I think you sh

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

logan-5 updated this revision to Diff 250130. logan-5 marked 3 inline comments as done. logan-5 added a comment. The check now suggests fixes for concrete (i.e. non-template) uses of ADL. Re-enabled the check for macros, and eliminated some false positives with arrays of dependent size. The chec

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

logan-5 marked an inline comment as done. logan-5 added inline comments. 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) + Quuxplusone wrote: > logan-5 wrote: > > E

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

On Wed, Mar 11, 2020 at 8:35 PM Arthur O'Dwyer via Phabricator < revi...@reviews.llvm.org> wrote: > Quuxplusone added inline comments. > > > > Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h:27 > +class UnintendedADLCheck : public ClangTidyCheck { > + const

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

Quuxplusone added inline comments. 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) + logan-5 wrote: > EricWF wrote: > > Arguably this is *exactly* the kind of code

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

logan-5 marked 2 inline comments as done. logan-5 added a comment. Thanks for the feedback. In D72282#1918253 , @EricWF wrote: > - This check should suggest fixes. It knows what function is actually being > resolved, and it has enough information to crea

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

Quuxplusone added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h:27 +class UnintendedADLCheck : public ClangTidyCheck { + const bool IgnoreOverloadedOperators; + const std::vector AllowedIdentifiers; EricWF wrote: > I th

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

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 ch

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

EricWF added a comment. I'll be picking this up seriously this week. I'm currently getting an internal version of this clang-tidy reviewed, and my plan was to submit it upstream after because I didn't see this patch until now (bad email filters). I think we can unify the two checks, Thanks for w

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

logan-5 added a comment. Just giving this a friendly ping! 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.o

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

aaron.ballman accepted this revision. aaron.ballman added reviewers: mclow.lists, EricWF. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, but I think it would be helpful if someone from the libc++ side also double-checked the behavior of patch if possible. I'

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

logan-5 updated this revision to Diff 243307. logan-5 marked an inline comment as done. logan-5 added a comment. Changed `Whitelist` to `AllowedIdentifiers`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72282/new/ https://reviews.llvm.org/D72282

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h:27 + const bool IgnoreOverloadedOperators; + const std::vector Whitelist; + I'd prefer a more descriptive name for this (and the user-facing option + do

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

logan-5 updated this revision to Diff 240691. logan-5 added a comment. Rebased with trunk. Updated whitelist to include more standard designated customization points . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

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

logan-5 updated this revision to Diff 238328. logan-5 added a comment. Added TODO comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72282/new/ https://reviews.llvm.org/D72282 Files: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.c

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

logan-5 marked an inline comment as done. logan-5 added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp:61 +void templateFunction(T t) { + swap(t, t); + Quuxplusone wrote: > logan-5 wrote: > > Quuxplusone wrot

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

Quuxplusone added subscribers: zoecarver, CaseyCarter, ldionne, EricWF, mclow.lists. Quuxplusone added a comment. Re which libc++ folks could give feedback on this ADL-diagnosing patch: I don't know precisely, but the candidates are few! @mclow.lists @ericwf @ldionne @zoecarver @CaseyCarter.

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

JonasToth added a comment. @Quuxplusone thank you for the input! I think it is a good idea if the library-implementors take a look at it. I myself don't have enough knowledge of C++ and the libraries to judge all this stuff. So who to ping or to add as reviewer? Comment at:

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

logan-5 updated this revision to Diff 236884. logan-5 marked 15 inline comments as done. logan-5 added a comment. Beefed up tests and split into separate files. Added tests for instantiations of template functions that //do// definitely result in ADL, and made the warning messages more descripti

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

logan-5 added a comment. Hi @Quuxplusone, glad you found your way here. I thought of adding you as a reviewer out the gate but then I didn't. Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:43 + Whitelist( + utils::options::parseStringLis

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

Quuxplusone added a comment. Repeating for emphasis: This is awesome. :) Having the opt-in //option// to not-ignore overloaded operators is good; please keep that option (in case anyone tries to talk you into removing it). For more than you wanted to know about ADL, why it's bad for library wri

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

Quuxplusone added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:43 + Whitelist( + utils::options::parseStringList(Options.get("Whitelist", "swap"))) {} + JonasToth wrote: > JonasToth wrote: > > logan-5 wro

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

JonasToth added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:43 + Whitelist( + utils::options::parseStringList(Options.get("Whitelist", "swap"))) {} + JonasToth wrote: > logan-5 wrote: > > JonasToth wrote

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

JonasToth added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:89 + +const auto *Lookup = +Result.Nodes.getNodeAs("templateADLexpr"); logan-5 wrote: > JonasToth wrote: > > Can't you just bind directly to the

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

logan-5 marked 2 inline comments as done. logan-5 added a comment. @JonasToth Thanks for the feedback. Will be updating the diff in the next day or so. Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:89 + +const auto *Lookup = +Result.Nodes

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

JonasToth added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:43 + Whitelist( + utils::options::parseStringList(Options.get("Whitelist", "swap"))) {} + logan-5 wrote: > JonasToth wrote: > > do you mean `st

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

logan-5 updated this revision to Diff 236593. logan-5 added a comment. Clean up some false positives in macro expansions and in expressions with nested potential ADL usages (e.g. in lambdas passed as function parameters). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:

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

logan-5 marked an inline comment as done. logan-5 added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:43 + Whitelist( + utils::options::parseStringList(Options.get("Whitelist", "swap"))) {} + JonasToth wro

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

JonasToth added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:43 + Whitelist( + utils::options::parseStringList(Options.get("Whitelist", "swap"))) {} + do you mean `std::swap`? If you it should be fully qu

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst:39 + + Semicolon-separated list of names that the check ignores. Default is `swap`. logan-5 wrote: > Eugene.Zelenko wrote: > > Indentation. P

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

logan-5 updated this revision to Diff 236420. logan-5 marked 5 inline comments as done. logan-5 added a comment. Addressed some formatting stuff. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72282/new/ https://reviews.llvm.org/D72282 Files: cla

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

logan-5 added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-unintended-adl.rst:35 + + If non-zero, ignores calls to overloaded operators using the operator syntax (e.g. `a + b`), but not the function call syntax (e.g. `operator+(a, b)`). Defa

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h:13 +#include "../ClangTidyCheck.h" + +#include Unnecessary empty line. Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-

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

logan-5 created this revision. logan-5 added reviewers: aaron.ballman, hokein, alexfh. logan-5 added a project: clang-tools-extra. Herald added subscribers: cfe-commits, xazax.hun, mgorny. Herald added a project: clang. This patch adds `bugprone-unintended-adl` which flags uses of ADL that are not