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

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked an inline comment as done.
balazske added inline comments.


================
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.
----------------
Szelethus wrote:
> 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?
The name of the function may be misleading. The non-empty `Optional` means "yes" and an empty means "no". And if yes additional information is returned in the optional value. If the transitions would be included in this function it needs more parameters. This function is for the part of the operation that computes the remaining error ( `getRemainingPossibleError` is a better name?). Later it will turn out in what way the function is used and if there is code repetition a new function can be introduced, but not now (the needed state transitions may be different).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75851