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

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

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();
----------------
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.


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1323
+/// pointer to the type of the constructed record or one of its bases.
+static bool hasNonTrivialConstructorCall(const CXXNewExpr *NE) {
 
----------------
Szelethus wrote:
> MTC wrote:
> > Szelethus wrote:
> > > MTC wrote:
> > > > I'm not sure `hasNonTrivialConstructorCall()` is a good name although you explained it in details in the comments below. And the comments for this function is difficult for me to understand, which is maybe my problem. 
> > > > 
> > > > And I also think this function doesn't do as much as described in the comments, it is just `true if the invoked constructor in \p NE has a parameter - pointer or reference to a record`, right?
> > > I admit that I copy-pasted this from the source I provided below, and I overlooked it before posting it here. I //very// much prefer what you proposed :)
> > The comments you provided is from the summary of the patch [[ https://reviews.llvm.org/D4025 | D4025 ]], ayartsev has not updated the summary information in time to adapt to his patch, so I think it is appropriate to extract the summary information when he submitted this patch, see [[ https://github.com/llvm-mirror/clang/commit/2e61d2893934f7b91ca9e2f889a4e4cc9939b05c#diff-ff9160d4628cb9b6186559837c2c8668 | [Analyzer] fix for PR19102 ]]. What do you think?
> Wow, nice detective work there :D Sounds good to me.
Umm, yea, I really couldn't come up with a better name. Hopefully the comments both here and at the call site help on this.


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

https://reviews.llvm.org/D54823