[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1087
   if (FD->getKind() == Decl::Function) {
-    initIdentifierInfo(C.getASTContext());
+    MemFunctionInfo.initIdentifierInfo(C.getASTContext());
     IdentifierInfo *FunI = FD->getIdentifier();
----------------
Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > MTC wrote:
> > > > Szelethus wrote:
> > > > > MTC wrote:
> > > > > > If I not wrong, `MemFunctionInfo` is mainly a class member of `MallocChecker` that hold a bunch of memory function identifiers and provide corresponding helper APIs. And we should pick a proper time to initialize it.
> > > > > > 
> > > > > > I want to known why you call `initIdentifierInfo()` in `checkPostStmt(const CallExpr *E,)`, this callback is called after `checkPreCall(const CallEvent &Call, )` in which we need `MemFunctionInfo`.
> > > > > Well, I'd like to know too! I think lazy initializing this struct creates more problems that it solves, but I wanted to draw a line somewhere in terms of how invasive I'd like to make this patch.
> > > > How about put the initialization stuff into constructor? Let the constructor do the initialization that it should do, let `register*()` do its registration, like setting `setOptimism` and so on.
> > > Interestingly, `MemFunctionInfo` is not always initialized, so a performance argument can be made here. I specifically checked whether there's any point in doing this hackery, and the answer is... well, some. I'll probably touch on these in a followup patch.
> > Lazy initialization of identifiers is kinda perceived as a fairly worthless optimization. Hence `CallDescription`.
> > 
> > Also it cannot be done during registration because the AST is not constructed yet at this point. Well, probably it can be done anyway because the empty identifier table is already there in the `ASTContext` and we can always eagerly add a few items into it, but it still sounds scary.
> I would personally prefer to risk initializing these during registration, but if we're this many comments into this discussion, then I believe it deserves a separate patch.
> 
> I'll leave a `TODO:` in the code.
Or just go with `CallDescription`.


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

https://reviews.llvm.org/D54823