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.
----------------
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.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:43
+ EofError, /// EOF condition (`feof` is true).
+ OtherError, /// Other (non-EOF) error (`ferror` is true).
+ AnyError /// EofError or OtherError, used if exact error is unknown.
----------------
balazske wrote:
> Szelethus wrote:
> > Shouldn't we call this `FError` instead, if this is set precisely when
> > `ferror()` is true? Despite the comments, I still managed to get myself
> > confused with it :)
> Plan is to rename to `NoError`, `FEofError`, `FErrorError`,
> `FEofOrFErrorError`.
If would suggest `NoError`, `FEoF`, `FError`, `FEoFOrFError`. Too many `Error`
suffixes reduce the readability.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:51
+
+ bool isNoError() const { return ErrorState == NoError; }
+ bool isEofError() const { return ErrorState == EofError; }
----------------
balazske wrote:
> Szelethus wrote:
> > If a stream's opening failed, its still an error, yet this getter would
> > return true for it. I think this is yet another reason to just merge the 2
> > enums :)
> But as the comment says, the error state is applicable only in the opened
> state. If not opened, we may not call the "error" functions at all (assertion
> can be added). Anyway it is possible to merge the enums, then the `isOpened`
> will be a bit difficult (true if state is one of `OpenedNoError`,
> `OpenedFErrorError`, `OpenedFEofError`, `OpenedFEofOrFErrorError`). Logically
> it is better to see that the state is an aggregation of two states, an
> "openedness" and an error state. Probably this does not matter much here
> because it is unlikely that new states will appear that make the code more
> complex.
I would not merge the two enums: this way they are hierarchical. When only
checking the openness we do not need to check for 4 different states. I
completely agree with @balazske here.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:54
+ bool isOtherError() const { return ErrorState == OtherError; }
+ bool isAnyError() const { return ErrorState == AnyError; }
+
----------------
Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > This is confusing -- I would expect a method called `isAnyError()` to
> > > return true when `isNoError()` is false.
> > The error state kinds are not mutually exclusive. The "any error" means we
> > know that there is some error but we do not know what the error is (it is
> > not relevant because nothing was done that depends on it). If a function is
> > called that needs to know if "ferror" or "feof" is the error, the state
> > will be split to `FErrorError` and `FEofError`.
> How about we change the name of it to `isUnknownError`? My main issue is that
> to me, the name `isAnyError()` answeres the question whether the stream is in
> any form of erroneous state.
Yes, "Any" usually means //any// of the possible kinds. But instead of
`UnknownError` use `FEoFOrFerror`.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:383
+
+ if (SS->isAnyError()) {
+ // We do not know yet what kind of error is set.
----------------
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.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:394
+ StreamSym, StreamState::getOpenedWithOtherError());
+ C.addTransition(StateEof);
+ C.addTransition(StateOther);
----------------
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.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:404-405
+
+void StreamChecker::evalFerror(const FnDescription *Desc, const CallEvent
&Call,
+ CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
----------------
balazske wrote:
> Szelethus wrote:
> > This function is practically the same as `evalFeof`, can we merge them?
> It is not the same code ("EOF" and "other error" are interchanged), but can
> be merged somehow.
Maybe using a template with a functor parameter and pass a lambda in the two
instantiations? (See `DebugContainerModeling` and `DebugIteratorModeling`) and
also some example is `Iterator.cpp`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75682/new/
https://reviews.llvm.org/D75682
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
