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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: include/clang/StaticAnalyzer/Core/CheckerManager.h:311
+                                  ExprEngine &Eng,
+                                  bool wasInlined = false);
+
----------------
dcoughlin wrote:
> Does 'wasInlined' really need to have a default argument? It looks like there are only two callers. (Also the capitalization is inconsistent).
That's copied from other methods (eg. `runCheckersForPostCall`), should i clean them up as well some day?


================
Comment at: lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp:143
+  /// \param Target The allocated region, casted to the class type.
+  void checkNewAllocator(const CXXNewExpr *NE, SVal Target,
+                         CheckerContext &) const {}
----------------
xazax.hun wrote:
> Is it possible to have a `NonLoc` Target? If no, would it be better to make it `Loc` type?
I guess it can be `UnknownVal` as well, or even `UndefinedVal`.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:343
+    if (const CXXNewExpr *CNE = dyn_cast_or_null<CXXNewExpr>(CE)) {
+      ExplodedNodeSet DstPostPostCallCallback;
+      getCheckerManager().runCheckersForPostCall(DstPostPostCallCallback,
----------------
dcoughlin wrote:
> 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()?
I think it's more readable inline. It's a small piece of code with subtle differences between copies (eg. here we have one predecessor node, there we have a set of nodes, then we need to pass the `WasInlined` flag), and also in `VisitCXXNewAllocatorCall()` all other checker callbacks are written out directly, so it doesn't make much sense to put these two into a sub-function while leaving three other callbacks in place - rather hurts readability. This code is kind of idiomatic and repeated many times in `ExprEngine` - maybe we should clean up the whole stuff, but i don't think cleaning up just this place is the right thing to do.


https://reviews.llvm.org/D41406