[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

Wed Jul 3 11:34:29 PDT 2019

NoQ added inline comments.

================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100
+// The APIModeling package is for checkers that model APIs. These checkers are
+// always turned on; this package is intended for API modeling that is not
+// controlled by the target triple.
----------------
Charusso wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > Charusso wrote:
> > > > Szelethus wrote:
> > > > > This isn't true: the user may decide to only enable non-pathsensitive checkers.
> > > > > 
> > > > > I think the comment should rather state that these whether these checkers are enabled shouldn't be explicitly specified, unless in **extreme** circumstances (causes a crash in a release build?).
> > > > Well, I have removed it instead. Makes no sense, you are right.
> > > I don't think it's a good idea -- we definitely should eventually be able to list packages with descriptions just like checkers (there actually is a FIXME in CheckerRegistry.cpp for that), but this is the next best thing that we have.
> > > 
> > > How about this:
> > > ```
> > > // The APIModeling package is for checkers that model APIs and don't perform
> > > // any diagnostics. Checkers within this package are enabled by the core or
> > > // through checker dependencies, so one shouldn't enable/disable them by
> > > // hand (unless they cause a crash, but that will cause dependent checkers to be
> > > // implicitly disabled).
> > > ```
> > I don't think any of these are dependencies. Most of the `apiModeling` checkers are there to suppress infeasible paths (exactly like this one).
> > 
> > I think i'd prefer to leave the comment as-is. We can always update it later.
> Thanks! Copy-pasted, just that patch produce diagnostics as notes.
Let's change to `don't emit any warnings` then.

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

https://reviews.llvm.org/D63915