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