Szelethus marked an inline comment as done.
Szelethus added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:107
+ /// This value applies to all error states in ErrorState except FEOF.
+ /// An EOF+indeterminate state is the same as EOF state.
+ bool FilePositionIndeterminate = false;
----------------
balazske wrote:
> Szelethus wrote:
> > Szelethus wrote:
> > > What does this mean? "An EOF+indeterminate state is the same as EOF
> > > state." I don't understand the message you want to convey here -- is it
> > > that we cannot have an indeterminate file position indicator if we hit
> > > EOF, hence we regard a stream that is **known** to be EOF to have its
> > > file position indicator determinate?
> > >
> > > A good followup question for the uninitiated would be that "Well why is
> > > it ever legal to construct a `StreamState` object that can both have the
> > > `FilePositionIndeterminate` set to true and the `ErrorState` indicate
> > > that the steam is **known** to be in EOF?" Well, the answer is that we
> > > may only realize later that the error state can only be EOF, like in here:
> > > ```lang=c++
> > > void f() {
> > > FILE *F = fopen(...);
> > > if (fseek(F, ...)) {
> > > // Could be either EOF, ERROR, and ofc indeterminate
> > > if (eof(F)) {
> > > // This is where we have a seemingly impossible stream state, but
> > > its not a programming error, its a design decision.
> > > }
> > > }
> > > ```
> > > This might warrant a bit on explanation either here, or in
> > > `ensureNoFilePositionIndeterminate`. Probably the latter.
> > >
> > > With that said, can `SteamState`'s constructor ensure that we do not
> > > create a known to be EOF stream that is indeterminate?
> > Actually, not enforcing this could leave to false positives:
> >
> > ```
> > void f() {
> > FILE *F = fopen(...);
> > if (fseek(F, ...)) {
> > // Could be either EOF, ERROR, and ofc indeterminate
> > if (eof(F)) {
> > clearerr(F);
> > fseek(F, ...); // false positive warning
> > }
> > }
> > ```
> The comment wants to say only that if the **ErrorState** contains the
> **ErrorFEof** the value of `filePositionIndeterminate` is to be ignored for
> the EOF case. If the file is in EOF it does not matter what value
> `filePositionIndeterminate` has. The cause for this handling is that
> ErrorState handles multiple possible errors together but the indeterminate
> position does not apply to all. If **ErrorState** contains **ErrorFEof** and
> **ErrorFError** together and the `filePositionIndeterminate` is set, the
> position is not indeterminate in the EOF case. For EOF case we should know
> that the position is at the end of the file, not indeterminate.
>
> Another solution for this problem can be to have a "ErrorFErrorIndeterminate"
> and "ErrorNoneIndeterminate" error type but this makes things more difficult
> to handle.
What do you mean under the term "the EOF case"? When we **know** the stream to
only be in the EOF state? The overall modeling seems correct, its just that
little corner case that if we **know** that the stream hit EOF, the file
position must be determinate.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:306
+ ProgramStateRef
+ ensureNoFilePositionIndeterminate(SVal StreamVal, CheckerContext &C,
+ ProgramStateRef State) const;
----------------
balazske wrote:
> Szelethus wrote:
> > Ooooor `ensureFilePositionDeterminate`? :D
> It is better to call "Invalid" or "Unknown" position, "indeterminate" is
> taken from the text of the C standard. I think "indeterminate" is a special
> name here that is better to have in this form always.
Sure, I'm sold.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80018/new/
https://reviews.llvm.org/D80018
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
