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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Yay. Wow. Cool. Thanks.

I heard that previously there was an explicit desire to avoid doxygen comments in checkers because they're quite useless because the checker is usually all in one file and doesn't have any clients that are interested in calling its methods directly.

But now that our checkers are split into files and start interacting with each other, this argument no longer holds.

So, well, thanks a lot.



================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:16
+//   * MallocChecker
+//       Despite it's name, it models all sorts of memory allocations and
+//       de- or reallocation, including but not limited to malloc, free,
----------------
~~it's~~ its


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:23
+//       MallocChecker, but do not enable MallocChecker's reports (more details
+//       to follow around it's field, ChecksEnabled).
+//       It also has a boolean "Optimistic" checker option, which if set to true
----------------
~~it's~~ its


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:283-300
+  mutable IdentifierInfo *II_alloca = nullptr, *II_win_alloca = nullptr,
+                         *II_malloc = nullptr, *II_free = nullptr,
+                         *II_realloc = nullptr, *II_calloc = nullptr,
+                         *II_valloc = nullptr, *II_reallocf = nullptr,
+                         *II_strndup = nullptr, *II_strdup = nullptr,
+                         *II_win_strdup = nullptr, *II_kmalloc = nullptr,
+                         *II_if_nameindex = nullptr,
----------------
Not now of course, but see also D45458. It'd be great to have call descriptions here, so that it looked like
```lang=c++
CallDescription alloca("alloca"),
                win_alloca("win_alloca"),
                malloc("malloc"),
                // ...
```
..and no initialization necessary later.

(we can even wrap them into a macro so that we didn't have to write names twice).


================
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
----------------
MTC wrote:
> `CE` typo?
Looks fine. It's the name of the parameter in the doxygen syntax.


================
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.
----------------
MTC wrote:
> `before realloction` typo?
Seems fine.


================
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:
> > 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.
> Interestingly, `MemFunctionInfo` is not always initialized, so a performance argument can be made here. I specifically checked whether there's any point in doing this hackery, and the answer is... well, some. I'll probably touch on these in a followup patch.
Lazy initialization of identifiers is kinda perceived as a fairly worthless optimization. Hence `CallDescription`.

Also it cannot be done during registration because the AST is not constructed yet at this point. Well, probably it can be done anyway because the empty identifier table is already there in the `ASTContext` and we can always eagerly add a few items into it, but it still sounds scary.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54823/new/

https://reviews.llvm.org/D54823