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

Wed Mar 11 07:58:42 PDT 2020

Szelethus added a comment.

I see that we're a bit stuck on naming, and that is largely my fault. Apologies, let us focus on high level stuff for a bit.

In D75682#1916435 <https://reviews.llvm.org/D75682#1916435>, @balazske wrote:

> I do not understand totally this remove-of-AnyError thing. If handling the `AnyError` will be removed the code remains still not testable.

In D75682#1915809 <https://reviews.llvm.org/D75682#1915809>, @Szelethus wrote:

> Remove the code that adds and handles `AnyError`. Merge the two enums in the `StreamState`, and make `ensureSteamOpened` emit a warning if the stream is either `feof()` or `ferror()`. Add `NoteTags` saying something along the lines of:
>
>   if (feof(F)) { // note: Assuming the condition is true
>                  // note: Stream 'F' has the EOF flag set
>     fgets(F); // warning: Illegal stream operation on a stream that has EOF set
>   }
>  
>

If we added warnings to this patch, that would be testable.

In D75682#1916809 <https://reviews.llvm.org/D75682#1916809>, @balazske wrote:

> What is better? Have this (or similar) change that adds a feature that is used in a later change and is only testable in that later change, or have a bigger change that contains real use of the added features? (This means: Add `fseek` to this change or not.) The error handling is not normally testable if there is no function that generates error state, `fseek` could be one.

Definitely the latter, and all you need to do is check whether the stream is in an error state in `ensureStreamOpened`, no need for fseek just yet.

In D75682#1916867 <https://reviews.llvm.org/D75682#1916867>, @baloghadamsoftware wrote:

> I think neither: add the feature (the last enum value and its handling in the functions) in the `fseek()` patch were it is used.

Yup.

================
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.
----------------
balazske wrote:
> xazax.hun wrote:
> > baloghadamsoftware wrote:
> > > 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.
> > In a more expressive language each enum value could have parameters and we could have
> > ```
> > Opened(ErrorKind),
> > Closed,
> > OpenFailed
> > ```
> > 
> > While we do not have such an expressive language, we can simulate this using the current constructs such as a variant. The question is, does this worth the effort? At this point this is more like the matter of taste as long as it is properly documented. 
> Have a `bool isOpened` and an error kind is better?
That sounds wonderful, @balazske! :)

================
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;
----------------
balazske wrote:
> 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.
Okay, sure.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682