balazske marked an inline comment as done. balazske added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:382-384 + C.addTransition(StateNotFailed); + C.addTransition(StateFailedWithFError); + C.addTransition(StateFailedWithoutFError); ---------------- baloghadamsoftware wrote: > Szelethus wrote: > > balazske wrote: > > > Szelethus wrote: > > > > This seems a bit excessive, we could merge the last two into > > > > `FSeekError`. > > > There are 3 cases: > > > - `fseek` did not fail at all. Return value is zero. This is > > > `StateNotFailed`. > > > - `fseek` failed but none of the error flags is true afterwards. Return > > > value is nonzero but `feof` and `ferror` are not true. This is > > > `StateFailedWithoutFError`. > > > - `fseek` failed and we have `feof` or `ferror` set (never both). Return > > > value is nonzero and `feof` or `ferror` will be true. This is > > > `StateFailedWithFError`. And an use of `AnyError`, otherwise we need here > > > 2 states, one for `feof` and one for `ferror`. But it is not important > > > here if `feof` or `ferror` is actually true, so the special value > > > `AnyError` is used and only one new state instead of two. > > Sure, but from the point of the analyzer the latter two are the same, isn't > > it? Its never a good idea to use a stream on which `fseek` failed without > > checking. > > > > State splits are the most expensive things the analyzer can make, which is > > why I'm cautious here. > Somehow, this should be done with just two new states. Maybe there should be > an error state "Unknown" instead of `OtherError` (or `FeoFOrFError` what I > suggested at the other patch) which can be any of the errors plus the > `NoError` value. Later, when `ferror()` of `feof()` is checked we can do a > second state split, but not earlier. Yes it is possible to use two new states only. For example by having a new `FSeekError` that can "manifest" in feof, ferror or no error. Better is to have only one "last-operation-failed-with-any-error" state and determine later if needed what errors are possible (based on the last operation). For `fseek` it is possible to get feof, ferror, or none of these (but still failed `fseek`). For reading operations the error can be feof or ferror. After write only ferror is possible. 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
