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

Thu Jan 9 10:42:50 PST 2020

logan-5 marked an inline comment as done.
logan-5 added inline comments.

================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp:61
+void templateFunction(T t) {
+  swap(t, t);
+
----------------
Quuxplusone wrote:
> logan-5 wrote:
> > 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.
> Juggling the tests around doesn't address the fact that any code that does `swap(a,b)` without doing `using std::swap;` first (or `begin(a)` without `using std::begin;`) is almost certainly broken for primitive types.
> 
> My naive thought is that you would //not// do `using std::make_error_code;` because `make_error_code` is definitely never going to be used with primitive types. So "functions okay to call via ADL" and "functions that require the std::swap two-step" actually are slightly different whitelists.
> 
> I was saying that although this issue is probably out-of-scope for what you're doing in this patch, still, it would be nice to leave a TODO somewhere. ...Or maybe you say "nah, that's so far out of scope I don't want to think about it, and it may never get done, so even leaving a TODO is inappropriate."
Thanks for clarifying. I do think that is out of scope for my goals for this patch, but I think a TODO comment is reasonable.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72282/new/

https://reviews.llvm.org/D72282