<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Mar 11, 2020 at 8:35 PM Arthur O'Dwyer via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Quuxplusone added inline comments.<br>
<br>
<br>
================<br>
Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h:27<br>
+class UnintendedADLCheck : public ClangTidyCheck {<br>
+  const bool IgnoreOverloadedOperators;<br>
+  const std::vector<std::string> AllowedIdentifiers;<br>
----------------<br>
EricWF wrote:<br>
> I think we should always ignore operators. I don't see value in having a mode where every comparison triggers this warning.<br>
> <br>
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>`](<a href="https://quuxplusone.github.io/blog/2019/09/26/uglification-doesnt-stop-adl/" rel="noreferrer" target="_blank">https://quuxplusone.github.io/blog/2019/09/26/uglification-doesnt-stop-adl/</a>), 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?<br></blockquote><div><br></div><div>I think the matcher you're describing this library to want is:<br><br></div><div>cxxOperatorCallExpr()</div><div><br></div><div>Because every instance of that node denotes a<span style="color:rgb(0,0,0);font-family:Roboto,sans-serif;font-size:14px"> call to an overloaded operator written using operator syntax.</span></div><div>This check doesn't need to indulge that strange use case.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
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.<br>
<br>
I think it would catch<br>
```<br>
template<class T> void foo(T t) { t, 0; }<br>
struct S { friend void operator,(S, int); };<br>
template void foo(S);<br>
```<br>
but not<br>
```<br>
template<class T> void foo(T t) { t, 0; }<br>
struct S { friend void operator,(S, int); };<br>
template void foo(int);<br>
```<br>
or<br>
```<br>
template<class T> void foo(T t) { t, 0; }<br>
```<br>
is that right?<br>
<br></blockquote><div><br></div><div>That's correct.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Repository:<br>
  rG LLVM Github Monorepo<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D72282/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D72282/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D72282" rel="noreferrer" target="_blank">https://reviews.llvm.org/D72282</a><br>
<br>
<br>
<br>
</blockquote></div></div>