baloghadamsoftware 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:
> > 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?
> That sounds wonderful, @balazske! :)
Yes, we do not need the `OpenFailed` state either. A stream is either opened or
not. This is the //state// of the stream. If there is any error, there are the
//error codes// for.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75682/new/
https://reviews.llvm.org/D75682
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
