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

[Artem Dergachev via Phabricator via cfe-commits]

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


================
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.
----------------
Charusso wrote:
> 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.
Well, that'd be the original code.

> I do not like `Optional<const T *>` anymore.

@Charusso, do you still plan to undo this change?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:88-93
+  Optional<const bool *> RawExpectedValue = CDM.lookup(Call);
+  if (!RawExpectedValue)
+    return;
+
+  SVal ReturnV = Call.getReturnValue();
+  bool ExpectedValue = **RawExpectedValue;
----------------
This can still be re-used by moving into `isInvariantBreak` (you can give it access to `CDM` by making it non-static).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:95-96
+  Optional<bool> IsInvariantBreak = isInvariantBreak(ExpectedValue, ReturnV, C);
+  if (!IsInvariantBreak)
+    return;
+
----------------
This looks flipped to me, should probably say `if (IsInvariantBreak) return;`.


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

https://reviews.llvm.org/D63915