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

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

Szelethus marked 3 inline comments 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();
----------------
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.


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1247
+    CheckerContext &C, const Expr *E, const unsigned IndexOfSizeArg,
+    ProgramStateRef State, Optional<SVal> RetVal) {
   if (!State)
----------------
MTC wrote:
> Szelethus wrote:
> > MTC wrote:
> > > `const` qualifier missing?
> > This method was made `static`.
> You are right, sorry for my oversight!
Actually, I forgot about it 10 times when I re-read the patch, no shame in that :)


================
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) {
 
----------------
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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54823