dcoughlin added a comment. This looks like a useful checker! Have you run this on large codebases yet? Does it find bugs? What kind of false positives do you see? Do you have a sense of what additional work would it take to bring this out of alpha and have it turned on by default?
Other than some super tiny comments in-line, this looks good to me. ================ Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:296 +def MisusedPolymorphicObjectChecker: Checker<"MisusedPolymorphicObject">, + HelpText<"Reports deletions of polymorphic objects with a non-virtual " ---------------- I think users would find it helpful if this had a more specific name. There are a lot of ways to misuse polymorphic objects, and this checker won't check all of them. What do you think about "DeleteWithNonVirtualDestructor"? ================ Comment at: lib/StaticAnalyzer/Checkers/MisusedPolymorphicObjectChecker.cpp:16 +// the last point where the derived-to-base conversion happened. +// +//===----------------------------------------------------------------------===// ---------------- I think it would be helpful to future maintainers to provide a high-level description of how this differs from `-Wnon-virtual-dtor` and `-Wdelete-non-virtual-dtor`. ================ Comment at: lib/StaticAnalyzer/Checkers/MisusedPolymorphicObjectChecker.cpp:137 + llvm::raw_svector_ostream OS(Buf); + OS << "Derived-to-base conversion happened here"; + PathDiagnosticLocation Pos(S, BRC.getSourceManager(), ---------------- A minor style suggestion to avoid the stacked noun phrase: "Conversion from derived to base happened here". https://reviews.llvm.org/D35796 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
