MTC added inline comments.
================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:342
+ DefaultBool IsOptimistic;
+ MemFunctionInfoTy MemFunctionInfo;
public:
----------------
Szelethus wrote:
> Szelethus wrote:
> > Szelethus wrote:
> > > MTC wrote:
> > > > I can't say that abstracting `MemFunctionInfo` is a perfect solution,
> > > > for example `IsOptimistic` and
> > > > `ShouldIncludeOwnershipAnnotatedFunctions ` are close to each other in
> > > > the object layout, but they do the same thing, which makes me feel
> > > > weird. And there are so many `MemFunctionInfo.` :)
> > > Well the thinking here was that `MallocChecker` is seriously overcrowded
> > > -- it's a one-tool-to-solve-everything class, and moving these
> > > `IdentifierInfos` in their separate class was pretty much the first step
> > > I took: I think it encapsulates a particular task well and eases a lot on
> > > `MallocChecker`. Sure `MemFunctionInfo.` is reduntant, but each time you
> > > see it, it indicates that we're calling a function or using a data member
> > > that has no effect on the actual analysis, that we're inquiring about
> > > more information about a given function. and nothing more. For a checker
> > > that emits warning at seemingly random places wherever, I think this is
> > > valuable.
> > >
> > > By the way, it also forces almost all similar conditions in a separate
> > > line, which is an unexpected, but pleasant help to the untrained eye:
> > > ```
> > > if (Fun == MemFunctionInfo.II_malloc ||
> > > Fun == MemFunctionInfo.II_whatever)
> > > ```
> > > Nevertheless, this is the only change I'm not 100% confident about, if
> > > you and/or others disagree, I'll happily revert it.
> > (leaving a separate inline for a separate topic)
> > The was this checker abuses checker options isn't nice at all, I agree. I
> > could just rename `MallocChecker::IsOptimistic` to
> > `MallocChecker::ShouldIncludeOwnershipAnnotatedFunctions`, and have it
> > passed around as a parameter in `MemFunctionInfoTy`. Would you prefer that,
> > should you be okay with `MemFunctionInfoTy` in general?
> The way* this checker abuses
Easier said than done :). I have no objection to this change, but I think merge
`IsOptimistic` and `ShouldIncludeOwnershipAnnotatedFunctions` together is a
good idea. This option seems from gabor, he may have new ideas.
================
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:
> 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.
================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1247
+ CheckerContext &C, const Expr *E, const unsigned IndexOfSizeArg,
+ ProgramStateRef State, Optional<SVal> RetVal) {
if (!State)
----------------
Szelethus wrote:
> MTC wrote:
> > `const` qualifier missing?
> This method was made `static`.
You are right, sorry for my oversight!
================
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:
> > 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?
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
