[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

Tue May 5 12:26:18 PDT 2020

Szelethus added inline comments.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:478
+
+  // FIXME: Check for stream in EOF state?
+
----------------
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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374