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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D63915#1563049 <https://reviews.llvm.org/D63915#1563049>, @NoQ wrote:

> Aha, nice, thanks for adding a description, it is a very good thing to do. Like, every commit should be obvious.


In some of my patches I have not added a description because they are so tiny changes.

> I guess this checker should eventually be merged with `StdCLibraryFunctionsChecker` and such.

Do you have any plans for that?



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


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:98
+  SVal RetV = CE.getReturnValue();
+  Optional<DefinedOrUnknownSVal> RetDV = RetV.getAs<DefinedOrUnknownSVal>();
+
----------------
NoQ wrote:
> Charusso wrote:
> > 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.
> I'm mildly interested, can you share a repro if you happen to still have one?
After the patch finishes I will make tests and try to break it again.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:39
+      // 'Error()'
+      {{{"ARMAsmParser", "Error"}}, true},
+      {{{"HexagonAsmParser", "Error"}}, true},
----------------
NoQ wrote:
> We can play even more nicely here by requiring namespace `llvm` as well (just prepend one more item to the list).
Well, I have checked 4 of them, they are in the anonymous namespace, so I will leave it.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:82
+
+        Out << '\'' << Name << "' always returns "
+            << (*Value ? "true" : "false");
----------------
NoQ wrote:
> Let's mention the class name as well! Maybe even the fully qualified namespace.
The class::call part would be tricky, because you need to hook what do you have in the CallDescription. It could be done with the decl-matching part, but then you have to rewrite the CallDescriptionMap interface as `lookup(), key(), value()`, so you could use the hooked info everywhere. It would require too much overhead for a print.


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

https://reviews.llvm.org/D63915