[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

[Kristóf Umann via Phabricator via cfe-commits]

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