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

Wed Jul 3 15:46:05 PDT 2019

Szelethus added a comment.

This checker isn't in alpha -- did you evaluate it on LLVM? Other than that, looks great!

================
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.
----------------
NoQ wrote:
> Charusso wrote:
> > NoQ wrote:
> > > 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?
> > Well, I am here at 2:1 against `Optional<>`, so I think it is your decision.
> I'd rather leave the original code as is and think more deeply about adding support for `Optional<const T &>` in the future.
Are you aware of this thread? http://lists.llvm.org/pipermail/cfe-dev/2018-July/058427.html

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

https://reviews.llvm.org/D63915