Szelethus added a comment.
And thank you for helping me along the way! :D
================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:342
+ DefaultBool IsOptimistic;
+ MemFunctionInfoTy MemFunctionInfo;
public:
----------------
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.
================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:342
+ DefaultBool IsOptimistic;
+ MemFunctionInfoTy MemFunctionInfo;
public:
----------------
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?
================
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:
> 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.
================
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:
> `const` qualifier missing?
This method was made `static`.
================
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:
> 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
:)
Repository:
rC Clang
https://reviews.llvm.org/D54823
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
