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

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

Szelethus added inline comments.


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


Repository:
  rC Clang

https://reviews.llvm.org/D54823