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