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