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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
