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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
