[PATCH] D80018: [Analyzer][StreamChecker] Added check for "indeterminate file position".

[Kristóf Umann via Phabricator via cfe-commits]

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;
----------------
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
    }
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80018/new/

https://reviews.llvm.org/D80018