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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
