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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits