[PATCH] D48521: [analyzer] Highlight STL object destruction in MallocChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Only minor nits, as usual :)



================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:483
       // Did not track -> released. Other state (allocated) -> released.
-      return (Stmt && (isa<CallExpr>(Stmt) || isa<CXXDeleteExpr>(Stmt)) &&
-              (S && S->isReleased()) && (!SPrev || !SPrev->isReleased()));
+      // The statement associated with the release might be missing.
+      return (S && S->isReleased()) && (!SPrev || !SPrev->isReleased());
----------------
xazax.hun wrote:
> To be honest, I am not sure what was the purpose of the AST based checks in the original version. IMHO, looking at the states only should be sufficient without examining the AST.
Yeah, same. I guess we could replace the check with an assertion (i.e. if there's statement, it's a call or delete-expression and it's `AF_InternalBuffer`), and see if anything crashes.


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2861
   const Stmt *S = PathDiagnosticLocation::getStmt(N);
-  if (!S)
+  // When dealing with STL containers, we sometimes want to give a note
+  // even if the statement is missing.
----------------
I don't think our check will be limited to STL containers only. We might cover other containers as well, such as LLVM or WebKit custom strings and string views.


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2914
+        default:
+          llvm_unreachable("unhandled allocation family");
+      }
----------------
xazax.hun wrote:
> I think these messages should be full sentences starting with a capital letter and ending with a period.
Or exclamantion mark! :)


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2991
+      return nullptr;
+    Pos = PathDiagnosticLocation(PostImplCall.getValue().getLocation(),
+                                 BRC.getSourceManager());
----------------
`PostImplCall->getLocation()`.


Repository:
  rC Clang

https://reviews.llvm.org/D48521