Szelethus added a comment.
You've sold me on `AnyError`, we should have something like that with the
addition of `NoteTags`. With that said, I feel uneasy about adding it in this
patch and not testing it properly. I can also sympathize with your anxiety
against bloating the `fseek` patch further by simply moving the code there, but
I'm just not sure there is a good way to avoid it. I have a suggestion:
1. 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
}
I think that would make this a very neat, self contained patch.
2. Add the removed code to the fseek patch (preferably with the name
`UnknownError`). Make `ensureSteamOpened` emit a warning on this enum value as
well. I don't think it would bloat the patch too much -- we just simply need
that much to make sense of it.
What do you think?
================
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:
> 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 :)
================
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:
> > 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`?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:51
+
+ bool isNoError() const { return ErrorState == NoError; }
+ bool isEofError() const { return ErrorState == EofError; }
----------------
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 :)
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:54
+ bool isOtherError() const { return ErrorState == OtherError; }
+ bool isAnyError() const { return ErrorState == AnyError; }
+
----------------
balazske wrote:
> Szelethus wrote:
> > This is confusing -- I would expect a method called `isAnyError()` to
> > return true when `isNoError()` is false.
> The error state kinds are not mutually exclusive. The "any error" means we
> know that there is some error but we do not know what the error is (it is not
> relevant because nothing was done that depends on it). If a function is
> called that needs to know if "ferror" or "feof" is the error, the state will
> be split to `FErrorError` and `FEofError`.
How about we change the name of it to `isUnknownError`? My main issue is that
to me, the name `isAnyError()` answeres the question whether the stream is in
any form of erroneous state.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:355
+
+ if (SS->isNoError())
+ return;
----------------
balazske wrote:
> Szelethus wrote:
> > 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)?
> `clearerr` does not return any value. It only clears the error flags (sets to
> false). In my interpretation the stream has one error value associated with
> it, this value may be **EOF** or "other" error, or no error. There are not
> two error flags, only one kind of error that may be EOF or other.
> The `clearerr()` function clears the end-of file and error indicators for the
> given stream.
Yup, you're totally right on this one. My bad.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75682/new/
https://reviews.llvm.org/D75682
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
