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