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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
