Szelethus marked 4 inline comments as done. Szelethus added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:378-379 + + using CheckFn = void (MallocChecker::*)(CheckerContext &C, const CallExpr *CE, + ProgramStateRef State) const; + ---------------- NoQ wrote: > 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. I'll upload followups where I'm addressing these issues -- to keep things simple, I decided against increasing this patch's scope. I won't commit until those are also accepted. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68165/new/ https://reviews.llvm.org/D68165 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
