[PATCH] D75356: [Analyzer][StreamChecker] Introduction of stream error state handling.

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:92-125
+class MakeRetVal {
+  const CallExpr *CE = nullptr;
+  std::unique_ptr<DefinedSVal> RetVal;
+  SymbolRef RetSym;
+
+public:
+  MakeRetVal(const CallEvent &Call, CheckerContext &C)
----------------
Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > balazske wrote:
> > > > Szelethus wrote:
> > > > > Do you have other patches that really crave the need for this class? Why isn't `CallEvent::getReturnValue` sufficient? This is a legitimate question, I really don't know. :)
> > > > This is an "interesting" solution for the problem that there is need for a function with 3 return values. The constructor performs the task of the function: Create a conjured value (and get the various objects for it). The output values are RetVal and RetSym, and the success state, and the call expr that is computed here anyway. It could be computed independently but if the value was retrieved once it is better to store it for later use. (I did not check how costly that operation is.)
> > > > 
> > > > I had some experience that using only `getReturnValue` and make constraints on that does not work as intended, and the reason can be that we need to bind a value for the call expr otherwise it is an unknown (undefined?) value (and not the conjured symbol)?
> > > I suspect that `getReturnValue` might only work in `postCall`, but I'm not sure.
> > > 
> > > I think instead of this class, a function returning a `std::tuple` would be nicer, with `std::tie` on the call site. You seem to use all 3 returns values in the functions that instantiate `MakeRetVal` anyways :).
> > > 
> > > In `StdLibraryFunctionsChecker`'s `evalCall`, the return value is [[https://github.com/llvm/llvm-project/blob/master/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp#L403|explicitly constructed]], and further constraints on it are only imposed in `postCall`. I wonder why that is. @martong, any idea why we don't `apply` the constraints for pure functions in `evalCall?`
> > The return value case is not as simple because the `DefinedSVal` has no default constructor, but it is not as bad to return only the `RetVal` and have a `CE` argument.
> I like the current solution very much!
> In StdLibraryFunctionsChecker's evalCall, the return value is explicitly constructed, and further constraints on it are only imposed in postCall. I wonder why that is. @martong, any idea why we don't apply the constraints for pure functions in evalCall?

We could apply them in evalCall technically.
I think the reason why we don't do that is the matter of implementation, and more importantly this way we are consequent with the traditional Hoare logic: {Pre}C{Post} as {Post} is done in postCall.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75356