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

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

Szelethus added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:37-40
+  // NoError: No error flag is set or stream is not open.
+  // EofError: EOF condition (feof returns true)
+  // OtherError: other (non-EOF) error (ferror returns true)
+  // AnyError: EofError or OtherError
----------------
balazske wrote:
> balazske wrote:
> > Szelethus wrote:
> > > balazske wrote:
> > > > Szelethus wrote:
> > > > > These too. Also, I'm not yet sure whether we need `OtherError` and `AnyError`, as stated in a later inline.
> > > > I plan to use `AnyError` for failing functions that can have **EOF** and other error as result. At a later `ferror` or `feof` call a new state split follows to differentiate the error (instead of having to add 3 new states after the function, for success, EOF error and other error). If other operation is called we know anyway that some error happened.
> > > I think it would still be better to introduce them as we find uses for them :) Also, to stay in the scope of this patch, shouldn't we only introduce `FseekError`? If we did, we could make warning messages a lot clearer.
> > This change is generally about introduction of the error handling, not specifically about `fseek`. Probably not `fseek` was the best choice, it could be any other function. Probably I can add another, or multiple ones, but then again the too-big-patch problem comes up. (If now the generic error states exist the diffs for adding more stream operations could be more simple by only adding new functions and not changing the `StreamState` at all.) (How is this related to warning messages?)
> For now, the `EofError` and `OtherError` can be removed, in this change these are not used (according to how `fseek` will be done).
> This change is generally about introduction of the error handling, not specifically about fseek. Probably not fseek was the best choice, it could be any other function.

You could not have picked a better function! Since the rules around the error state of the stream after a failed fseek call are quite complex, few functions deserve their own error state more.

> (How is this related to warning messages?)

I had the following image in my head, it could be used at the bug report emission site to give a precise description:

```lang=cpp
  if (SS->isInErrorState()) {
    switch(SS.getErrorKind) {
    case FseekError:
      reportBug("After a failed fseek call, the state of the stream may "
                "have changed, and it might be feof() or ferror()!");
      break;
    case EofError:
      reportBug("The stream is in an feof() state!");
      break;
    case Errorf:
      reportBug("The stream is in an ferror() state!");
      break;
    case OtherError:
      // We don't know what the precise error is, but we surely
      // know its in one.
      reportBug("The stream is in an error state!");
      break;
    }
```
> (If now the generic error states exist the diffs for adding more stream operations could be more simple by only adding new functions and not changing the StreamState at all.)

For the last case in the code snippet (`OtherError`), I'm not too sure what the conditions are -- when do we know **what** the stream state is (some sort of an error), but not know precisely **how**? In the case of `fseek`, we don't precisely know **what** the state is but we know **how** it came about. I just don't yet see why we need a generic error state.

> Probably I can add another, or multiple ones, but then again the too-big-patch problem comes up.

I think the point of the patch is to demonstrate the implementation of an error state, not to implement them all, and it does it quite well!

> For now, the EofError and OtherError can be removed, in this change these are not used (according to how fseek will be done).

I agree!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75356