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

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

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

LGTM! You seem to have a firm grasp on where you want this checker to go, and I like everything about it.

The code needs seme `clang-format` treatment, and another eye might not hurt, but as far as I'm concerned, I'm super happy how this patch turned out.



================
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.
----------------
balazske wrote:
> 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).
`getRemainingPossibleError` sounds great! You convinced me, if we ever need to stuff more things into this function, we will do that then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75851