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

[Eric Fiselier via cfe-commits]

On Wed, Mar 11, 2020 at 8:35 PM Arthur O'Dwyer via Phabricator <
reviews at 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200311/4cfc6623/attachment-0001.html>