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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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

> 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.

I think the benefits from the mimicked std library outweigh the problems from trying to test against a moving target in multiple dimensions, and so I think we want to keep the mimicked std library approach as the default for tests. However, I also think the lack of testing against real world STL implementations is a problem that hurts us, for all the reasons pointed out already. Ultimately, the problem with testing against a real STL will boil down to who is responsible for changes when a test case starts failing after the check author has moved on. However, we might be able to solve this with policy decisions like changing a test to an expected failure + bug report while waiting for someone to step up to solve the issue. But this is orthogonal to your patch and would require a larger community discussion.

In D95714#2533046 <https://reviews.llvm.org/D95714#2533046>, @steveire wrote:

> I think you should be able to correctly match everything. Try a matcher like (can probably be cleaned up a bit):

Thank you for that matcher suggestion; you are a powerful wizard. :-)

Generally, the changes LGTM.



================
Comment at: clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.cpp:58
+                // that if an implicit cast is found, it is not from another
+                // nested rewritten operator
+                expr().bind("matchBinopOperands"),
----------------



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

https://reviews.llvm.org/D95714