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

[Csaba Dabis via Phabricator via cfe-commits]

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


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:145
+
+  // Get the concrete return value.
+  Optional<const bool *> RawConcreteValue = CDM.lookup(*Call);
----------------
NoQ wrote:
> 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.
Great advice, thanks you!


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

https://reviews.llvm.org/D63915