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