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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
