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

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 2 inline comments as done.
balazske added a comment.

I do not understand totally this remove-of-AnyError thing. If handling the `AnyError` will be removed the code remains still not testable. Because none of the possible failing file operations are modeled in this revision. A test with code like `if (feof(F)) {` can not work because `feof(F)` is never true without a previously failed operation. (We decide not at the moment of calling `feof` that `feof` will return true, it is decided at a previously failed operation, together with the return value of that operation.)



================
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;
----------------
Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > 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`.
> > This is the problem is the change is too small: The next part of it introduces functions where the new error flags are used. In this patch the AnyError feature is not used (the feof and ferror read it but nothing sets it).
> Okay, after your comment in the other revision, I see how such a kind should be left, and the bug report should rather be enriched by `NoteTags`. With that said, `AnyError` is still a bit confusing as a name -- how abour `UnknownError`?
I want it to rename to `FEofOrFErrorError`, this tells exactly what it is (`UnknownError` can be still thought as a third error type). For these 2 error types this naming is not too long, and new error types are not likely to appear here later.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:51
+
+  bool isNoError() const { return ErrorState == NoError; }
+  bool isEofError() const { return ErrorState == EofError; }
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75682/new/

https://reviews.llvm.org/D75682