[PATCH] D41406: [analyzer] Add a new checker callback, check::NewAllocator.

[Devin Coughlin via Phabricator via cfe-commits]

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