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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
