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

[Umann Kristóf via Phabricator via cfe-commits]

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