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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D63915#1568166 <https://reviews.llvm.org/D63915#1568166>, @Szelethus wrote:

> This checker seems to only check LLVM functions, but doesn't check whether these methods lie in the LLVM namespace. Is this intended?


Thanks for the reviews! They are not in the `llvm` namespace.



================
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.
----------------
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.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1119
 
-  const T *lookup(const CallEvent &Call) const {
+  Optional<T> lookup(const CallEvent &Call) const {
     // Slow path: linear lookup.
----------------
Szelethus wrote:
> Charusso wrote:
> > NoQ wrote:
> > > I hope `T` never gets too expensive to copy. The ideal return value here is `Optional<const T &>` but i suspect that `llvm::Optional`s don't support this (while C++17 `std::optional`s do). Could you double-check my vague memories here?
> > Optional<const T *> is working and used widely, I like that.
> Why do we need the optional AND the pointer? How about we just return with a nullptr if we fail to find the call?
`Optional<>` stands for optional values, that is why it is made. @NoQ tried to avoid it, but I believe if someone does not use it for an optional value, that break some kind of unspoken standard.


================
Comment at: clang/test/Analysis/return-value-guaranteed.cpp:90
+
+    // no-warning: "The left operand of '==' is a garbage value" was here.
+    doSomething();
----------------
Szelethus wrote:
> Was it? I just tried it out and it doesn't seem to be the case.
Whoops, too heavy copy-pasting.


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

https://reviews.llvm.org/D63915