[PATCH] D95714: [clang-tidy] fix modernize-use-nullptr false positive with spaceship operator comparisons

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc added a comment.

In D95714#2532565 <https://reviews.llvm.org/D95714#2532565>, @njames93 wrote:

> This does highlight an issue where the mimicked std library stubs used for tests don't correspond exactly to what the stdlib actually looks like and can result in subtly different ASTs that have added/removed implicit nodes.
>
> Going a little off point here but a few months back I pushed a fix for another check that passed its tests. 
> However the bug report was re-opened as the bug was still observable in the real word. 
> Turned out the implementation of std::string used for the test had a trivial destructor resulting in the AST not needed to emit `CXXBindTemporaryExpr`s all over the place, which threw off the matching logic.
>
> Unfortunately this kind of disparity is hard to detect in tests so it may be wise to test this locally using the compare header from a real standard library implementation (preferably all 3 main ones if you have the machines) and see if this behaviour is correct.

Indeed the //mimicked std library// approach has real advantages like running fast and giving consistent results across platforms. But it also has drawbacks like the replication of //mimicked std// code across tests, and possible differences between test and real world behaviour. We could give tests a `CLANG_TIDY_TEST_NATIVE_STD` macro to switch between //mimicked std// code and real headers, and set up CI to test both ways, to catch these issues at the expense of added complexity. But that's a broader discussion.

Back to `modernize-use-nullptr`, these days I'm working in MSVC and the tests pass with MSVC's `<compare>`, with my mimicked compare, and a shorter mimicked `strong_ordering` that I found in `ASTTraverserTest.cpp`. But I fetched gcc's `<compare>` from https://raw.githubusercontent.com/gcc-mirror/gcc/master/libstdc%2B%2B-v3/libsupc%2B%2B/compare and now see the same ASTs as you, and the proposed fix fails.

Thoughts on which option seems like the best path forward?

1. In the ASTs I've seen so far the third child of the `CXXRewrittenBinaryOperator`'s `CXXOperatorCallExpr ` is the one to ignore. But comments in <clang\AST\ExprCXX.h>:268-283 imply that perhaps sometimes the `0` may appear first? There should be some way to determine which child was added by the `CXXRewrittenBinaryOperator` rewrite and skip that child, regardless of `std` implementation, but I have yet to figure that out.
2. Roll back to `hasAncestor`, fixing the immediate problem of creating invalid fixes (`a1 nullptr 2a`) and accepting that some rare but fixable code (`a1 < ( ptr == 0 ? a2 : a3`) will not be modernized to `nullptr`.
3. Change the bugfix approach here from AST-based to text-based. In `replaceWithNullptr` in `UseNullptrCheck.cpp:62`, add code to skip the planned replacement if the text we're about to replace with `nullptr` consists of `<`, `=`, and `>`. As a quick test, this version seems to make all tests pass and doesn't depend on std header implementation:

  static const std::string SuspiciousChars("<=>");
  char FirstCharToReplace = *SM.getCharacterData(StartLoc);
  char LastCharToReplace = *SM.getCharacterData(EndLoc);
  if (SuspiciousChars.find(FirstCharToReplace) != std::string::npos &&
      SuspiciousChars.find(LastCharToReplace) != std::string::npos)
    return;


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

https://reviews.llvm.org/D95714