balazske marked 2 inline comments as done.
balazske 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:
> > > 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.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:383
+ // Record the failed status, only if failed.
+ // fseek clears the EOF flag, sets only error flag.
+ StateFailed = StateFailed->set<StreamErrorMap>(RV.getRetSym(),
----------------
Szelethus wrote:
> According to the C'98 standard
> [[http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf|§7.19.9.2.5]]:
> > After determining the new position, a successful call to the fseek function
> > undoes any effects of the `ungetc` function on the stream, clears the
> > end-of-file indicator for the stream, and then establishes the new
> > position. After a successful fseek call, the next operation on an update
> > stream may be either input or output.
>
> So it definitely doesn't clear the `EOF` flag on failure.
Yes it does say nothing about what happens with **EOF** flag on failure, so it
should be is better to not change it. And we do not know if it is possible to
get an **EOF** error (seek to after the end of file?).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75356/new/
https://reviews.llvm.org/D75356
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
