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