JonasToth added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:43
+ Whitelist(
+ utils::options::parseStringList(Options.get("Whitelist", "swap"))) {}
+
----------------
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 :)
================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:83
+ Call = Result.Nodes.getNodeAs<CallExpr>("templateADLcall");
+ assert(Call);
+
----------------
please add a message to the assertion to describe why you are asserting this
condition, to better understand the cause of the error, e.g. `assert(Call &&
"Matcher should only match for two distinct cases");` or something like this.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:89
+
+ const auto *Lookup =
+ Result.Nodes.getNodeAs<UnresolvedLookupExpr>("templateADLexpr");
----------------
Can't you just bind directly to the `unresolvedExpr`?
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp:1
+// RUN: %check_clang_tidy -std=c++14-or-later %s bugprone-unintended-adl %t
+
----------------
why 14 or later? `ADL` exists in the prior standards, too.
Additionally we need a test for where overloaded operators are not ignored and
create the warnings.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp:29
+ ops::stream << 5;
+ operator<<(ops::stream, 5);
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: expression calls
'ops::operator<<' through ADL [bugprone-unintended-adl]
----------------
Templated overloaded operators should be tested, too, e.g.:
```
template <class IStream>
OStream &operator>>(IStream& is, MyClass foo);
```
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp:60
+template <typename T>
+void templateFunction(T t) {
+ swap(t, t);
----------------
This function is not instantiated right now, is it?
Can you please write a function that would resolve to ADL for one instantiation
and wouldn't for another one?
That should create multiple diagnostics for the same source-location and would
be confusing.
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
