balazske marked 2 inline comments as done. balazske added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:478 + + // FIXME: Check for stream in EOF state? + ---------------- Szelethus wrote: > balazske wrote: > > Szelethus wrote: > > > Will that be the next patch? :D Eager to see it! > > It may be too big change to add it as well here (or cause more > > difficulties). But it should be a check for `ErrorFEof` in the > > `ErrorState` (and add another bug type). This is not a fatal error. > > > > It may be more difficult to make `fread` return the feof error again if it > > starts already with feof state. (Because the state with FEof should be > > split from the generic error state that can contain other possible errors.) > > It may be too big change to add it as well here (or cause more > > difficulties). But it should be a check for ErrorFEof in the ErrorState > > (and add another bug type). This is not a fatal error. > > Oh, yea, right, we went over this once :) What I really meant, is an EOF > related error, like reading a variable with an EOF value in any context that > isn't a comparison to the actual `EOF` (which in many contexts still isn't a > //fatal// error, but is far more an indicator of code smell). But I'm just > thinking aloud, please proceed with this project however it is convenient to > you! > > > It may be more difficult to make fread return the feof error again if it > > starts already with feof state. (Because the state with FEof should be > > split from the generic error state that can contain other possible errors.) > > I gave this plenty of thought, how do you imagine the problem of us not > splitting up to 3 states to show up? Suppose we're calling fread on a stream > where the error state is either EOF or ERROR, but we don't know which. We > could just leave the `StreamErrorState` as-is, couldn't we? > reading a variable with an EOF value in any context that isn't a comparison > to the actual EOF This could be another checker, or it can be integrated somehow into `ErrorReturnChecker` (that does something similar already). I mean, remembering if a variable contains **EOF** that comes from a function that may return **EOF** (otherwise the program can do nothing with a simple -1 numerical value without getting the warning), then if any other thing is done with it than comparison to **EOF** (or passing it to other function) it can be an error case. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:478 + + // FIXME: Check for stream in EOF state? + ---------------- balazske wrote: > Szelethus wrote: > > balazske wrote: > > > Szelethus wrote: > > > > Will that be the next patch? :D Eager to see it! > > > It may be too big change to add it as well here (or cause more > > > difficulties). But it should be a check for `ErrorFEof` in the > > > `ErrorState` (and add another bug type). This is not a fatal error. > > > > > > It may be more difficult to make `fread` return the feof error again if > > > it starts already with feof state. (Because the state with FEof should be > > > split from the generic error state that can contain other possible > > > errors.) > > > It may be too big change to add it as well here (or cause more > > > difficulties). But it should be a check for ErrorFEof in the ErrorState > > > (and add another bug type). This is not a fatal error. > > > > Oh, yea, right, we went over this once :) What I really meant, is an EOF > > related error, like reading a variable with an EOF value in any context > > that isn't a comparison to the actual `EOF` (which in many contexts still > > isn't a //fatal// error, but is far more an indicator of code smell). But > > I'm just thinking aloud, please proceed with this project however it is > > convenient to you! > > > > > It may be more difficult to make fread return the feof error again if it > > > starts already with feof state. (Because the state with FEof should be > > > split from the generic error state that can contain other possible > > > errors.) > > > > I gave this plenty of thought, how do you imagine the problem of us not > > splitting up to 3 states to show up? Suppose we're calling fread on a > > stream where the error state is either EOF or ERROR, but we don't know > > which. We could just leave the `StreamErrorState` as-is, couldn't we? > > reading a variable with an EOF value in any context that isn't a comparison > > to the actual EOF > This could be another checker, or it can be integrated somehow into > `ErrorReturnChecker` (that does something similar already). I mean, > remembering if a variable contains **EOF** that comes from a function that > may return **EOF** (otherwise the program can do nothing with a simple -1 > numerical value without getting the warning), then if any other thing is done > with it than comparison to **EOF** (or passing it to other function) it can > be an error case. > Suppose we're calling fread on a stream where the error state is either EOF > or ERROR, but we don't know which. We could just leave the StreamErrorState > as-is, couldn't we? * If `fread` is called with **FEOF** flag, it returns 0 and **FEOF** remains. * If `fread` is called with **FERROR** flag, it may fail (with any error) or not. This is similar as calling `fread` in no-error state. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78374/new/ https://reviews.llvm.org/D78374 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
