[PATCH] D75851: [Analyzer][StreamChecker] Added evaluation of fseek.

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

Szelethus marked an inline comment as done.
Szelethus added a comment.

In D75851#1933538 <https://reviews.llvm.org/D75851#1933538>, @balazske wrote:

> Simplified code.


I can tell! The patch is amazing, the code practically reads itself aloud. I have some inlines but nothing major, the high level idea is great.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:103-110
+  /// Return if the specified error kind is possible on the stream in the
+  /// current state.
+  /// This depends on the stored `LastOperation` value.
+  /// If the error is not possible returns empty value.
+  /// If the error is possible returns the remaining possible error type
+  /// (after taking out `ErrorKind`). If a single error is possible it will
+  /// return that value, otherwise unknown error.
----------------
This feels clunky to me.

Suppose that we're curious about the stream reaching `EOF`. We know that the last operation on the stream could cause `EOF` and `ERROR`. This function then would return `FError`. That doesn't really feel like the answer to the question "isErrorPossible".

In what contexts do you see calling this function that isn't like this?:

```lang=c++
  Optional<StreamState::ErrorKindTy> NewError =
      SS->isErrorPossible(ErrorKind);
  if (!NewError) {
    // This kind of error is not possible, function returns zero.
    // Error state remains unknown.
    AddTransition(bindInt(0, State, C, CE), StreamState::UnknownError);
  } else {
    // Add state with true returned.
    AddTransition(bindAndAssumeTrue(State, C, CE), ErrorKind);
    // Add state with false returned and the new remaining error type.
    AddTransition(bindInt(0, State, C, CE), *NewError);
  }
```

Would it make more sense to move the logic of creating state transitions into a function instead?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:518
 
-  if ((SS->*IsOfError)()) {
-    // Function returns nonzero.
-    DefinedSVal RetVal = makeRetVal(C, CE);
-    State = State->BindExpr(CE, C.getLocationContext(), RetVal);
-    State = State->assume(RetVal, true);
-    assert(State && "Assumption on new value should not fail.");
+  if (!SS->isUnknownError()) {
+    // We know exactly what the error is.
----------------
It took me a while to realize that this also includes the case where the stat is not in any error :) Could you add a line of comment please?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:534-537
+      // Add state with true returned.
+      AddTransition(bindAndAssumeTrue(State, C, CE), ErrorKind);
+      // Add state with false returned and the new remaining error type.
+      AddTransition(bindInt(0, State, C, CE), *NewError);
----------------
I gave this plenty of thought, and I now believe that the state split here is appropriate. We really should ensure that the user checked both `feof` and `ferror`.


================
Comment at: clang/test/Analysis/stream-error.c:48
+  if (rc) {
+    int Eof = feof(F), Error = ferror(F);
+    // Get feof or ferror or no error.
----------------
That is a bit misleading, because `Eof` isn't `EOF` as specified in the standard. How about `IsEof`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75851