dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
Looks good to me with some minor nits inside (and also a request to consider
factoring out common code).
================
Comment at: include/clang/StaticAnalyzer/Core/CheckerManager.h:311
+ ExprEngine &Eng,
+ bool wasInlined = false);
+
----------------
Does 'wasInlined' really need to have a default argument? It looks like there
are only two callers. (Also the capitalization is inconsistent).
================
Comment at: lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp:138
+ /// new-expression is this pointer. This callback is called between steps
+ /// (2) and (3). Post-call for the allocator is called before step (1).
+ /// Pre-statement for the new-expression is called on step (4) when the value
----------------
I find the following confusing: "Post-call for the allocator is called before
step (1)." Did you mean "after" step one?
================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:296
+ ProgramStateRef State,
+ Optional<SVal> RetVal = None) const;
----------------
It is probably worth explaining what RetVal is and what it means if it is None
in the doxygen.
================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:324
+ AllocationFamily Family = AF_Malloc,
+ Optional<SVal> RetVal = None);
----------------
Same comment about documenting RetVal here.
================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:964
+ CheckerContext &C, const Expr *E, const unsigned AllocationSizeArg,
+ ProgramStateRef State, Optional<SVal> retVal) const {
if (!State)
----------------
Nit: Capitalization of "retVal".
================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1289
+ AllocationFamily Family,
+ Optional<SVal> retVal) {
if (!State)
----------------
Capitalization, again.
================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1302
+ SymbolRef Sym = retVal->getAsLocSymbol();
assert(Sym);
----------------
I think it is worth a comment here about why we expect this assert succeed: the
value will always be the result of a malloc function or an un-inlined call to
the new allocator (if I understand correctly).
================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:343
+ if (const CXXNewExpr *CNE = dyn_cast_or_null<CXXNewExpr>(CE)) {
+ ExplodedNodeSet DstPostPostCallCallback;
+ getCheckerManager().runCheckersForPostCall(DstPostPostCallCallback,
----------------
Is it possible/desirable to factor out this logic (running the PostCall
callbacks and then the NewAllocator callbacks) into a helper method on
ExprEngine then call the helper from both VisitCXXNewAllocatorCall() and
processCallExit()?
https://reviews.llvm.org/D41406
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
