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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

I think i like it now!



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


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:229
+  /// @param Cb Callback with 'BugReporterContext &, BugReport &' parameters.
+  /// @param IsPrunable Whether the note is prunable.
+  const NoteTag *getNoteTag(NoteTag::Callback &&Cb, bool IsPrunable = false) {
----------------
"Allow BugReporter to omit the note from the report if it would make the displayed bug path significantly shorter."


================
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;
----------------
Charusso wrote:
> NoQ wrote:
> > This can still be re-used by moving into `isInvariantBreak` (you can give it access to `CDM` by making it non-static).
> Well, sadly not. In both of the checks it is used inside the call, so you cannot just remove it. I really wanted to make it as simple as possible, but you know, "not simpler".
Aha, ok, right!


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:95-96
+  Optional<bool> IsInvariantBreak = isInvariantBreak(ExpectedValue, ReturnV, C);
+  if (!IsInvariantBreak)
+    return;
+
----------------
Charusso wrote:
> NoQ wrote:
> > This looks flipped to me, should probably say `if (IsInvariantBreak) return;`.
> It is the `Optional<>` checking, whether we could obtain the value. I really wanted to write `!hasValue()`, but no one use that, so it is some kind of unspoken standard to just `!` it.
Mmm. Aha. Ok. Indeed. Sry^^

I was thinking about simply err-ing towards "it's not a break" when we don't know for sure that it's a break, because in this case we have no problems with taking the branch that we want.

But your code seems to be more careful and i like it :)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:69
+  Name += Call.getCalleeIdentifier()->getName();
+  return Name;
+}
----------------
Charusso wrote:
> xazax.hun wrote:
> > `CXXMethodDecl::getQualifiedNameAsString` is not doing what you want here?
> We want to drop the namespaces for better readability.
Yeah, i think it's important to display exactly what we match for.

We might eventually do some sort of `CallDescription.explain()` (and then maybe even `CallDescriptionMap.explain(Call)`) for that purpose.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:154
+
+        BR.markInteresting(SFC);
+
----------------
Hmm. It is enough to set `IsPrunable` to `false`; once you do that, there's actually no need to mark the stack frame as interesting. I guess this wasn't really necessary.


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

https://reviews.llvm.org/D63915