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

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

Szelethus 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:
> > 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.
> I think an APIModeling could not be too much thing, most of the stuff around it is not commented out what they do. But as @Szelethus really wanted to inject that, I cannot say no to a copy-paste.
Some are, but saying something along the lines of "most of these are enabled by default by the Driver" would've been more correct, but yea, let's leave this for another day.


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

https://reviews.llvm.org/D63915