Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed.
I happen to have very recent analyses on a couple projects, I'll throw this in: LLVM+Clang+Clang-tools-extra <http://cc.elte.hu:15001/Default/#is-unique=on&run=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions%20WITH%20moderate%20tracking&review-status=Unreviewed&review-status=Confirmed&detection-status=New&detection-status=Reopened&detection-status=Unresolved&checker-name=optin.cplusplus.VirtualCall&tab=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions%20WITH%20moderate%20tracking>. No findings on Xerces or Bitcoin. I caught up on the thread you linked, and the checker code since it wasn't that long. Overall, what you'd like to do with this checker sounds amazing! I agree that the visitor does more harm then good, and that pure only analysis should be on-by-default. Also, thanks for cleaning up the testfile!! > I'd like to point out that it breaks backwards compatibility. I'm not going > to be hurt by this and i haven't heard a lot about users of this opt-in check > but if you know them, please let me know. If we agree to break backwards > compatibility, we should make sure that the result is the best, so i'd like > to hear @Szelethus's opinion, as he usually has strong opinions on checker > options and dependencies :) CodeChecker turns on `optin.cplusplus.VirtualCall` by default, so this wouldn't hurt its users much. Also, you can enable/disable checkers from an arbitrary version of clang, their names aren't hardcoded (only the on-by-default list). That said, dependencywise, there is a directed circle here, despite `optin.cplusplus.VirtualCall` being a glorified checker option (this is what I like to label as a "subchecker"). [side note: since this checker doesn't really do any modeling, in fact it wouldn even store a state if it inspected the call stack, it wouldn't be logical to create a modeling checker that would be a dependency of both `PureVirtualCall` and `VirtualCall`. Since `PureVirtualCall` is a strict subset of `VirtualCall`, if it causes any crashes, you have to disable both anyways. ] Your current implementation would cause an assertion failure if you enabled both checkers due to attempting to register the checker object 2 times, to avoid that, *see inlines* <------------------ / \ PureVirtualCall VirtualCall and should be: PureVirtualCall <------ VirtualCall \ / ------------------> ================ Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:562 + HelpText<"Check virtual function calls during construction/destruction">, Documentation<HasDocumentation>; ---------------- `Dependencies<[PureVirtualCallChecker]>,` ================ Comment at: clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp:210 void ento::registerVirtualCallChecker(CheckerManager &mgr) { VirtualCallChecker *checker = mgr.registerChecker<VirtualCallChecker>(); + checker->IsPureOnly = false; ---------------- `auto *Checker = mgr.getChecker<VirtualCallChecker>();` Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64274/new/ https://reviews.llvm.org/D64274 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
