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 wrote:
> > > JonasToth wrote:
> > > > do you mean `std::swap`? If you it should be fully qualified.
> > > > Doesn't `std::error_code` rely on adl, too? I think `std::cout <<` and 
> > > > other streams of the STL rely on it too, and probably many more 
> > > > code-constructs that are commonly used.
> > > > 
> > > > That means, the list should be extended to at least all 
> > > > standard-library facilities that basically required ADL to work. And 
> > > > then we need data on different code bases (e.g. LLVM is a good start) 
> > > > how much noise gets generated.
> > > I distinctly //don't// mean `std::swap` -- I want to whitelist any 
> > > unqualified function call spelled simply `swap`.
> > > 
> > > Overloaded operators are the poster child for ADL's usefulness, so that's 
> > > why this check has a special default-on `IgnoreOverloadedOperators` 
> > > option. That whitelists a whole ton of legitimate stuff including 
> > > `std::cout << x` and friends.
> > > 
> > > I don't see a ton of discussion online about 
> > > `error_code`/`make_error_code` and ADL being very much intertwined. I'm 
> > > not particularly familiar with those constructs myself though, and I 
> > > could just be out of the loop. I do see a fair number of unqualified 
> > > calls to `make_error_code` within LLVM, though most of those resolve to 
> > > `llvm::make_error_code`, the documentation for which says it exists 
> > > because `std::make_error_code` can't be reliably/portably used with ADL. 
> > > That makes me think `make_error_code` would belong in LLVM's 
> > > project-specific whitelist configuration, not the check's default.
> > > 
> > > Speaking of which, I did run this check over LLVM while developing and 
> > > found it not particularly noisy as written. That is, it generated a fair 
> > > number of warnings, but only on constructs that, when examined closely, 
> > > //were// a little suspicious or non-obvious.
> > I don't have a solid understanding of the `error_code` world as well. All I 
> > know is, that you specialize some templates with your own types in order to 
> > use the generic `error_code`-world.
> > AFAIK that needs some form of ADL at some point, but that could even happen 
> > through the overloaded operators (`==` and `!=`), in which case that would 
> > already be handled. (maybe @aaron.ballman knows more?)
> > 
> > But overloaded operators being ignored by default is good and that point is 
> > gone :)
> Yes, `make_error_code` is used via ADL. --> 
> https://www.boost.org/doc/libs/1_72_0/libs/outcome/doc/html/motivation/plug_error_code.html
> I think that should be in the default list for ignored functions, as it is a 
> standard facility.
+1, both `make_error_code` and `make_error_condition` should be on the 
whitelist. (I am the author of [P0824 "Summary of SG14 discussion of 
`<system_error>`"](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0824r1.html#best-arthur).
 I also confirm that libc++ `<system_error>` does call them unqualified on 
purpose.)

I would like to see some discussion and/or TODO-comments about the other 
standard [designated customization 
points](http://eel.is/c++draft/iterator.range#1.sentence-2): `data`, `begin`, 
`end`, `rbegin`, `rend`, `crbegin`, `crend`, `size`, `ssize`, and `empty`. This 
might deserve input from the libc++ implementors.


================
Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp:54
+  move(a);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls 'ns::move' 
through ADL [bugprone-unintended-adl]
+  forward(a);
----------------
This is awesome. :)  Please also consider ADL where there's more than one 
associated namespace, something like this: https://godbolt.org/z/S73Gzy
```
template<class T> struct Templated { };
Templated<std::byte> testX() {
    Templated<std::byte> x;
    using std::move;
    return move(x);  // "correctly" resolves to std::move today, but still does 
unintended ADL
}
```
Please add a test isomorphic to the above, unless you think it's already 
covered by one of the existing tests.


================
Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp:61
+void templateFunction(T t) {
+  swap(t, t);
+
----------------
This is not the idiomatic way of calling `swap`: there is no ADL swap for 
`int`, for example (so `templateFunction<int>` will hard-error during 
instantiation). It would probably be scope-creep to try to handle the 
"std::swap two-step", but can you leave a TODO comment somewhere to revisit 
this issue?



================
Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp:69
+
+  ref(t);
+  ::ref(t);
----------------
Ah, this is subtle. Lookup on `ref` finds the global variable `ref` and 
therefore doesn't do ADL. But if global variable `ref` didn't exist, then this 
would warn: `std::ref` is not an ADL customization point.

Could you either leave a explanatory comment here, or (if the subtle confusion 
with `std::ref` is not germane to the test case) `s/ref/foo/`?


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

Reply via email to