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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
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());
----------------
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.


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


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2988
+  if (!S) {
+    auto PostImplCall = N->getLocation().getAs<PostImplicitCall>();
+    if (!PostImplCall.hasValue())
----------------
This new code should only be used for `AF_InternalBuffer` allocation family? If that is the case, maybe adding an assert here and running it on some large codebases to se if this is triggered would be great.


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2989
+    auto PostImplCall = N->getLocation().getAs<PostImplicitCall>();
+    if (!PostImplCall.hasValue())
+      return nullptr;
----------------
LLVM's optional can implicitly convert to bool. I think it is perfectly fine to rely on the implicit conversion here.


Repository:
  rC Clang

https://reviews.llvm.org/D48521