https://github.com/NagyDonat commented:
This is a _very_ sophisticated checker that ensures correct and user-friendly behavior even in tricky corner cases. (Even among the "big" core checkers, there are many that were written with less attention to details.) In addition to extensive test coverage, this checker was evaluated on `llvm-project` and other big C++ projects, and didn't produce crashes or too much noise, which demonstrates that it is very close to use in production. As I reviewed the code, I added many inline suggestions, but most of them are just minor readability suggestions where the original code is also correct. (There is, however, one tricky corner case where I think using function pointers could trigger a nullpointer dereference.) The way this checker defers reports (to report user-written code instead of standard library internals) is a nice innovation, perhaps later it could be applied in other checkers as well. However, I think it would be nice if the diagnostic message would indicate that the destructor is called indirectly, through a standard library utility. The current message is fitting for `std::destroy_at` and `std::default_delete`, but the message could be surprising if it was coming from the destructor of a standard type (e.g. a `vector` or similar container). Try to add a testcase where the placement new reinitializes some part of a `std` object and compose a message that also fits this situation. (Note: there are some heuristics in the analyzer that prevent the inlining of some `std` utilities or suppress reports coming from there. Look into these if your tests behave in an unexpected way.) Most of the listed limitations seem to be very reasonable, many core checkers have more severe limitations than this. My only question is the limitation > explicit non-virtual base class destructor call on a derived object is > reported, although [class.dtor]/16 allows it. What was the obstacle to following the standard in this case? (At first glance it seems that this is significantly easier than some of the difficulties you already solved while developing this checker. Btw the checker is OK even with this limitation and would be OK with several more limitations.) https://github.com/llvm/llvm-project/pull/237801 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
