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

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

Thank you for doing the cleaning that no one else is interested in! Comments is inline below.



================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:342
+  DefaultBool IsOptimistic;
+  MemFunctionInfoTy MemFunctionInfo;
 public:
----------------
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.` :)


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:422
+  ///
+  /// \param [in] CE The expression that allocates memory.
+  /// \param [in] IndexOfSizeArg Index of the argument that specifies the size
----------------
`CE` typo?


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:428
+  ///   if unspecified, the value of expression \p E is used.
+  static ProgramStateRef ProcessZeroAllocation(CheckerContext &C, const Expr *E,
+                                               const unsigned IndexOfSizeArg,
----------------
Personally, `ProcessZeroAllocation` is like half the story, maybe `ProcessZeroAllocCheck` or `ProcessZeroAllocationCheck` is better.


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:448
+  /// \param [in] State The \c ProgramState right before allocation.
+  /// \returns The programstate right after allocation.
   ProgramStateRef MallocMemReturnsAttr(CheckerContext &C,
----------------
Maybe `program state` or just `ProgramState` is better?
Same as 462, 476, 507, 575, 593.


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:592
+  /// \param [in] CE The expression that reallocated memory
+  /// \param [in] State The \c ProgramState right before reallocation.
+  /// \returns The programstate right after allocation.
----------------
`before realloction` typo?


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1087
   if (FD->getKind() == Decl::Function) {
-    initIdentifierInfo(C.getASTContext());
+    MemFunctionInfo.initIdentifierInfo(C.getASTContext());
     IdentifierInfo *FunI = FD->getIdentifier();
----------------
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`.


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1247
+    CheckerContext &C, const Expr *E, const unsigned IndexOfSizeArg,
+    ProgramStateRef State, Optional<SVal> RetVal) {
   if (!State)
----------------
`const` qualifier missing?


================
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) {
 
----------------
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?


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2407
                                              const CallExpr *CE,
-                                             bool FreesOnFail,
+                                             bool ShouldFreeOnFail,
                                              ProgramStateRef State,
----------------
The parameter name `ShouldFreeOnFail` is not consistent with the declaration, see line 577.


Repository:
  rC Clang

https://reviews.llvm.org/D54823