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 bool IgnoreOverloadedOperators;
> + const std::vector<std::string> AllowedIdentifiers;
> ----------------
> EricWF wrote:
> > I think we should always ignore operators. I don't see value in having a
> mode where every comparison triggers this warning.
> >
> I think there's value in that mode, for library writers (not libc++) who
> really care about finding every unintentional ADL in their whole library.
> The standard library is designed not-to-work with types like
> [`Holder<Incomplete>`](
> https://quuxplusone.github.io/blog/2019/09/26/uglification-doesnt-stop-adl/),
> but someone else's library might be designed to work even in that case, and
> then they'd want to hear about ADL lookups for things like `operator,`.
> Besides, it's just 1 extra line of code in the patch, isn't it?
>
I think the matcher you're describing this library to want is:
cxxOperatorCallExpr()
Because every instance of that node denotes a call to an overloaded
operator written using operator syntax.
This check doesn't need to indulge that strange use case.
> However, I now think I may not understand how this check works. I thought
> it looked for unqualified calls (even in templates) that "may" use ADL, but
> now that I look again at the tests, it seems to trigger only on concrete
> calls (in concrete template instantiations) that "do" use ADL, which sounds
> still useful but much less comprehensive than I had thought.
>
> I think it would catch
> ```
> template<class T> void foo(T t) { t, 0; }
> struct S { friend void operator,(S, int); };
> template void foo(S);
> ```
> but not
> ```
> template<class T> void foo(T t) { t, 0; }
> struct S { friend void operator,(S, int); };
> template void foo(int);
> ```
> or
> ```
> template<class T> void foo(T t) { t, 0; }
> ```
> is that right?
>
>
That's correct.
>
> 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
