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

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

balazske marked 4 inline comments as done.
balazske 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.
----------------
xazax.hun wrote:
> baloghadamsoftware wrote:
> > xazax.hun wrote:
> > > 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.
> > Not orthogonal, but rather hiearchical. That is a reason for not merging. I am completely against it.
> In a more expressive language each enum value could have parameters and we could have
> ```
> Opened(ErrorKind),
> Closed,
> OpenFailed
> ```
> 
> While we do not have such an expressive language, we can simulate this using the current constructs such as a variant. The question is, does this worth the effort? At this point this is more like the matter of taste as long as it is properly documented. 
Have a `bool isOpened` and an error kind is better?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682