[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I think my `strdup` comments weren't addressed >.<



================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:378-379
+
+  using CheckFn = void (MallocChecker::*)(CheckerContext &C, const CallExpr *CE,
+                                          ProgramStateRef State) const;
+
----------------
baloghadamsoftware wrote:
> Szelethus wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > Whenever i see a (`CE`, `State`) pair, it screams `CallEvent` to me. That said, i'm worried that `State` in these callbacks isn't necessarily equal to `C.getState()` (the latter, by the way, is always equal to the `CallEvent`'s `.getState()` - that's a relief, right?), so if you'll ever be in the mood to check that, that'd be great :)
> > > It should be always equal to it. I'll change it.
> > Hmmm, I tried making this take a (`CallEvent`, `CheckerContext`), but it bloated the code for little gain, since every handler function would start with the retrieval of the state and the call expression. That said, I could cascade these down to `FreeMemAux`, `MallocMemAux`, etc, to also take this pair instead, but I'll be honest, I don't see point until something actually breaks.
> This is the standard way in the checkers: almost every handler function starts with the retrieval of the state from the `CheckerContext`. The only reason for an extra `State` parameter is that sometimes we create more states in the lower level functions but only add the final one to the `CheckerContext` as a new transition. Does something like this happen here?
I agree with @baloghadamsoftware here. If you pass `State` explicitly, it looks like an indication for the reader that this `State` may be different from `C.getState()`. If in practice they're the same, i'd rather do `C.getState()` in every method than confuse the reader.

Also do you remember what makes us query `CallExpr` so often? Given that `CallEvent` includes `CallExpr`, we should be able to expose everything we need as `CallEvent` methods. Say, we should be able to replace `MallocMemAux(C, CE, CE->getArg(0), ...)` with `MallocMemAux(C, Call, Call.getArgExpr(0), ...)` and only do `Call.getOriginExpr()` once when we report a bug.


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

https://reviews.llvm.org/D68165