[PATCH] D75432: [analyzer][NFC][MallocChecker] Convert many parameters into CallEvent

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Must have been annoying, thanks a lot!



================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:424
   /// new-expression that goes before the constructor.
-  void processNewAllocation(const CXXNewExpr *NE, CheckerContext &C,
-                            SVal Target, AllocationFamily Family) const;
+  LLVM_NODISCARD
+  ProgramStateRef processNewAllocation(const CXXAllocatorCall &Call,
----------------
Excellent, we should totally do this more often wherever necessary.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:614
+  static ProgramStateRef CallocMem(CheckerContext &C, const CallEvent &Call,
                                    ProgramStateRef State);
 
----------------
Is the state parameter still necessary here?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:948
   // as well as Linux kmalloc.
-  if (CE->getNumArgs() < 2)
+  if (Call.getNumArgs() < 2)
     return None;
----------------
`CallDescriptionMap` was supposed to verify this for us.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:995
 
-void MallocChecker::checkBasicAlloc(CheckerContext &C, const CallExpr *CE,
-                                    ProgramStateRef State) const {
-  State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State, AF_Malloc);
-  State = ProcessZeroAllocCheck(C, CE, 0, State);
+void MallocChecker::checkBasicAlloc(const CallEvent &Call,
+                                    CheckerContext &C) const {
----------------
Szelethus wrote:
> xazax.hun wrote:
> > What is the main reason of getting rid of the state parameter? I think this could make using these functions more error prone overall as it would be easier to introduce splits in the exploded graph this way (if we somehow wanted to model a function as successive calls of some other functions).
> Refer to @NoQ's [[https://reviews.llvm.org/D68165?id=222257#inline-612842|earlier inline]].
> 
> > I think this could make using these functions more error prone overall as it would be easier to introduce splits in the exploded graph this way (if we somehow wanted to model a function as successive calls of some other functions).
> 
> Why would the removal of a `ProgramStateRef` parameter cause that?
> if we somehow wanted to model a function as successive calls of some other functions

If we ever want to compose our evaluations, we should do it as a sequence of composable operations on a state, i.e. by not only //accepting// the state but also //returning// the state.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2769-2770
 
-  for (const Expr *Arg : CE->arguments())
+  for (const Expr *Arg : cast<CallExpr>(Call.getOriginExpr())->arguments())
     if (SymbolRef Sym = C.getSVal(Arg).getAsSymbol())
       if (const RefState *RS = State->get<RegionState>(Sym))
----------------
Maybe iterate over `CallEvent`'s argument `SVal`s directly? We probably don't have a method for this but it doesn't sound like a too uncommon of a task.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75432