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::parseStringList(Options.get("Whitelist", "swap"))) {} + ---------------- Quuxplusone wrote: > 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. Added `make_error_condition` to the whitelist. My inclination would be to just add all those standard customization points to the default whitelist. Users can easily supply a smaller whitelist if they want. ================ 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); ---------------- Quuxplusone wrote: > 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. It's interesting, that code only triggers the check (i.e. my AST matchers only think it's doing ADL) without the `using std::move`. I admit I'm a bit confused as to why. ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp:61 +void templateFunction(T t) { + swap(t, t); + ---------------- Quuxplusone wrote: > 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? > I believe this addressed by my juggling the tests around a bit. 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