[PATCH] D138701: [clang-tidy] Ignore cxxRewrittenBinaryOperator in defaulted function decls in modernize-use-nullptr.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!
Initially I thought that traversing with `TK_IgnoreUnlessSpelledInSource` might be appropriate. But that idea does not seem to work as we do need to match implicit casts to find the `nullptr`.

I am also not sure if we should show this warning when the generated code was written by hand. The check will keep suggesting to replace `(a <=> b) < 0` with `(a <=> b) < nullptr`.
Technically this will compile, but the C++ Standard explicitly mentions the comparisons must be implicitly rewritten to `(a <=> b) < 0`, not `nullptr`.
But let's not solve this problem until someone actually reports it.



================
Comment at: clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp:66
+                        .bind(CastSequence))),
+                unless(hasAncestor(functionDecl(isDefaulted()))))));
 }
----------------
NIT: maybe add a comment mentioning that this tries to find defaulted comparison operators.
Pre-C++20 one could only put `= default` constructors, destructors and assignment operators. So I suspect this might be a useful clarification for readers of the code.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize/use-nullptr-cxx20.cpp:6
+
+struct _CmpUnspecifiedParam {
+  consteval
----------------
NIT: add a comment mentioning that this mocks how STL defined the parameter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138701