[PATCH] D118880: [analyzer] Improve NoOwnershipChangeVisitor's understanding of deallocators

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I like it.
nits here and there;



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h:102
 
+  /// Returns true if the CallEvent is a call to a function that matches
+  /// the CallDescription.
----------------
CallExpr


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:750
   SymbolRef Sym;
+  const MallocChecker *Checker;
   using OwnerSet = llvm::SmallPtrSet<const MemRegion *, 8>;
----------------
What about having a const reference, or a const pointer to const?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:802
 
+  bool isFreeingCallImprecise(const CallExpr &Call) const {
+    if (Checker->FreeingMemFnMap.lookupImprecise(Call) ||
----------------
You should probably add an example of when is it imprecise and an explanation of why we have this function.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:807
+
+    if (const auto *Func = dyn_cast<FunctionDecl>(Call.getCalleeDecl()))
+      return MallocChecker::isFreeingOwnershipAttrCall(Func);
----------------



================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:818-819
       return false;
-    // TODO: Operator delete is hardly the only deallocator -- Can we reuse
+    // TODO: Operator delete is hardly the only deallocatr -- Can we reuse
     // isFreeingCall() or something thats already here?
+    auto Matches = match(findAll(stmt(anyOf(cxxDeleteExpr().bind("delete"),
----------------
I guess, this gets fixed by this patch.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:820-831
+    auto Matches = match(findAll(stmt(anyOf(cxxDeleteExpr().bind("delete"),
+                                            callExpr().bind("call")))),
+                         *FD->getBody(), ACtx);
+    for (BoundNodes Match : Matches) {
+      if (Match.getNodeAs<CXXDeleteExpr>("delete")) {
+        return true;
+      }
----------------
Without braces, it would be slightly more compact & readable IMO.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:832
+    }
     // TODO: Ownership my change with an attempt to store the allocated memory.
+    return false;
----------------
Could you clarify what this wants to denote? I'm just curious.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1104
+
+  if (const auto *Func = dyn_cast<FunctionDecl>(Call.getDecl()))
+    return isFreeingOwnershipAttrCall(Func);
----------------



================
Comment at: clang/test/Analysis/NewDeleteLeaks.cpp:152
+  // like to deallocate anything.
+  bar();
+}
----------------
I guess we would not have the warning if `bar` would accept the pointer `P`, since that way it would escape.
Am I correct?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118880/new/

https://reviews.llvm.org/D118880