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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:98
+  SVal RetV = CE.getReturnValue();
+  Optional<DefinedOrUnknownSVal> RetDV = RetV.getAs<DefinedOrUnknownSVal>();
+
----------------
NoQ wrote:
> `castAs` if you're sure.
> 
> Generally it's either evaluated conservatively (so the return value is a conjured symbol) or inlined (and in this case returning an undefined value would result in a fatal warning), so return values shouldn't ever be undefined.
> 
> The only way to obtain an undefined return value is by binding it in `evalCall()`, but even then, i'd rather issue a warning immediately.
Sometimes it failed with live tests in the wild.


================
Comment at: clang/test/Analysis/return-value-guaranteed.cpp:20
+struct Foo { int Field; };
+bool error();
+bool problem();
----------------
NoQ wrote:
> NoQ wrote:
> > `core.builtin.NoReturnFunctions` reacts on this function. You can easily see it in the explodedgraph dump, as the last sink node in `test_calls::parseFoo` is tagged with that checker. You'll have to pick a different name for testing purposes.
> (yes, this is why your tests aren't working)
> (see also D63965)
I was not sure why do you mention an unrelated patch, but I realized as the previous graph-rewriter patches, it has my stuff in the summary. That would be the last thing I considered. It is a nice catch, thanks!


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

https://reviews.llvm.org/D63915