[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Henry Wong via Phabricator via cfe-commits]

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