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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:33-45
+  enum KindTy {
+    Opened, /// Stream is opened.
+    Closed, /// Closed stream (an invalid stream pointer after it was closed).
+    OpenFailed /// The last open operation has failed.
+  } State;
+
+  /// The error state of a stream.
----------------
Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > Hmm, now that I think of it, could we just merge these 2 enums? Also, I fear that indexers would accidentally assign the comment to the enum after the comma:
> > > 
> > > ```lang=cpp
> > >     Opened, /// Stream is opened.
> > >     Closed, /// Closed stream (an invalid stream pointer after it was closed).
> > >     OpenFailed /// The last open operation has failed.
> > > ```
> > > ` /// Stream is opened` might be assigned to `Closed`. How about this:
> > > ```lang=cpp
> > >     /// Stream is opened.
> > >     Opened,
> > >     /// Closed stream (an invalid stream pointer after it was closed).
> > >     Closed,
> > >     /// The last open operation has failed.
> > >     OpenFailed
> > > ```
> > Probably these can be merged, it is used for a stream that is in an indeterminate state after failed `freopen`, but practically it is handled the same way as a closed stream. But this change would be done in another revision.
> I meant to merge the two enums (`StreamState` and `ErrorKindTy`) and the fields associated with them (`State` and `ErrorState`). We could however merge `Closed` and `OpenFailed`, granted that we put a `NoteTag` to `evalFreopen`. But I agree, that should be another revision's topic :)
Since you mentioned that ErrorState is only applicable to Open streams, I am also +1 on merging the enums. These two states are not orthogonal, no reason for them to be separate.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:394
+        StreamSym, StreamState::getOpenedWithOtherError());
+    C.addTransition(StateEof);
+    C.addTransition(StateOther);
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682