Szelethus added a comment.
Can we see more test cases for when after a call to `feof` we indeed can tell
the stream is in an `EOF` state? Same for `ferror`. I feel like there is a lot
of code we don't yet cover.
Also, I see now that I didn't get in earlier revisions that `OtherError`
actually refers to `ferror()` -- sorry about the confusion :)
================
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.
----------------
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
```
================
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.
----------------
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 :)
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:44
+ OtherError, /// Other (non-EOF) error (`ferror` is true).
+ AnyError /// EofError or OtherError, used if exact error is unknown.
+ } ErrorState = NoError;
----------------
When do we know that the stream is in an error state, but not precisely know
what that error is within the context of this patch? `fseek` would indeed
introduce such a state, as described in D75356#inline-689287, but that should
introduce its own error `ErrorKind`. I still don't really get why we would ever
need `AnyError`.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:53
+ bool isEofError() const { return ErrorState == EofError; }
+ bool isOtherError() const { return ErrorState == OtherError; }
+ bool isAnyError() const { return ErrorState == AnyError; }
----------------
Same, shouldn't this be `isFError()`?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:54
+ bool isOtherError() const { return ErrorState == OtherError; }
+ bool isAnyError() const { return ErrorState == AnyError; }
+
----------------
This is confusing -- I would expect a method called `isAnyError()` to return
true when `isNoError()` is false.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:355
+
+ if (SS->isNoError())
+ return;
----------------
What if we call `clearerr()` on a stream that is in an `feof()` state? Shouln't
we return if the stream is `!isOtherError()` (or `!isFError()`, if we were to
rename it)?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:383
+
+ if (SS->isAnyError()) {
+ // We do not know yet what kind of error is set.
----------------
Does this ever run? We never actually set the stream state to `AnyError`.
================
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();
----------------
This function is practically the same as `evalFeof`, can we merge them?
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
