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

Sat Jan 30 18:02:15 PST 2021

njames93 added a comment.

In D95714#2532516 <https://reviews.llvm.org/D95714#2532516>, @poelmanc wrote:

> @njames93 Thank you so much for the quick feedback, I made your suggested changes and added a test that it properly converts `result = (a1 > (ptr == 0 ? a1 : a2));` to `result = (a1 > (ptr == nullptr ? a1 : a2));` now.
> This looks a little bit different from your AST but I'm not sure what code your AST was generated from. If you have a test case I'd be happy to take a look. Thanks!

I used the same test code as yours, but rather than creating my own `std::strong_ordering` I included the compare header. For reference I was using a trunk clang build with a trunk version of libstdc++ - https://godbolt.org/z/PrjhbK

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.

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

https://reviews.llvm.org/D95714