[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/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:107
+
+	// If the invariant is broken we do not return the concrete value.
+        if (IsInvariantBreak)
----------------
Something's wrong with formatting.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:109
+        if (IsInvariantBreak)
+          Out << " should return ";
+        else
----------------
I'd still like to avoid "should". We are in no position to teach them what should their method return. It's not a matter of convention; it's a matter of correctness. We don't want people to go fix their code to return true when they see a note. We only need to point out that the return value is not what they expect.

I suggest removing this note entirely, i.e., early-return when we see an invariant break in `checkPostCall`, because we've already emitted a note in `checkEndFunction`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:145
+
+  // Get the concrete return value.
+  Optional<const bool *> RawConcreteValue = CDM.lookup(*Call);
----------------
NoQ wrote:
> I think you can squeeze all this code into one big `isInvariantBreak(Call, ReturnV)` and re-use it across both methods.
I think it would be more precise to talk about "expected"/"actual" return value. The actual return value may be either concrete (i.e., `nonloc::ConcreteInt`) or symbolic (i.e., `nonloc::SymbolVal`).

Also, there's a relatively famous rule of a thumb for making good comments: "comments shouldn't explain what does the code do, but rather why does it do it". Instead of these comments i'd rather have one large comment at the beginning of `checkEndFunction` explaining what are we trying to do here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:145-157
+  // Get the concrete return value.
+  Optional<const bool *> RawConcreteValue = CDM.lookup(*Call);
+  if (!RawConcreteValue)
+    return;
+ 
+  // Get the symbolic return value.
+  SVal ReturnV = State->getSVal(RS->getRetValue(), C.getLocationContext());
----------------
I think you can squeeze all this code into one big `isInvariantBreak(Call, ReturnV)` and re-use it across both methods.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:170
+
+	// The following is swapped because the invariant is broken.
+        Out << '\'' << Name << "' returns "
----------------
Something's wrong with formatting.


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

https://reviews.llvm.org/D63915