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