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

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 2 inline comments as done.
balazske added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:394
+        StreamSym, StreamState::getOpenedWithOtherError());
+    C.addTransition(StateEof);
+    C.addTransition(StateOther);
----------------
baloghadamsoftware wrote:
> xazax.hun wrote:
> > As you probably know we are splitting the execution paths here. This will potentially result in doubling the analysis tome for a function for each `feof` call. Since state splits can be really bad for performance we need good justification for each of them.
> > So the questions are:
> > * Is it really important to differentiate between eof and other error or is it possible to just have an any error state?
> > * Do we find more bugs in real world code that justifies these state splits?
> I agree here. Do not introduce such forks without specific reason.
`feof` and `ferror` should return the same value if called multiple times (and no other file operations in between). If the exact error is not saved in the state, how can we ensure that the calls return the same value?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:404-405
+
+void StreamChecker::evalFerror(const FnDescription *Desc, const CallEvent &Call,
+                               CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
----------------
baloghadamsoftware wrote:
> balazske wrote:
> > Szelethus wrote:
> > > This function is practically the same as `evalFeof`, can we merge them?
> > It is not the same code ("EOF" and "other error" are interchanged), but can be merged somehow.
> Maybe using a template with a functor parameter and pass a lambda in the two instantiations? (See `DebugContainerModeling` and `DebugIteratorModeling`) and also some example is `Iterator.cpp`.
I have already a template implementation, but probably it is not needed now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682