[PATCH] D35796: [analyzer] Misused polymorphic object checker

[Devin Coughlin via Phabricator via cfe-commits]

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