balazske marked 9 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.
----------------
baloghadamsoftware wrote:
> 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.
The "error kind" concept is needed separately from the stream state, see
D75851. This is a reason for not merging them.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:383
+
+ if (SS->isAnyError()) {
+ // We do not know yet what kind of error is set.
----------------
baloghadamsoftware wrote:
> Szelethus wrote:
> > Does this ever run? We never actually set the stream state to `AnyError`.
> I suggest to move codes that are only executed after a new functionality is
> added in a subsequent patch to that patch.
This is not applicable any more, `AnyError` is removed (from here).
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:394
+ StreamSym, StreamState::getOpenedWithOtherError());
+ C.addTransition(StateEof);
+ C.addTransition(StateOther);
----------------
Szelethus wrote:
> balazske wrote:
> > 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?
> This is a very interesting question indeed, never crossed my mind before. I
> think it would be better to move `AnyError` to the next revision and discuss
> it there.
The state fork problem is solved with `UnknownError` is the later revision,
here it is no problem any more. A single state split is always needed because
the return values differ in the failed and success case, and I want to make
correlation between return values and stream error state.
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
