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

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 3 inline comments as done.
balazske added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:207-210
+      {{"fread", 4},
+       {&StreamChecker::preFread,
+        std::bind(&StreamChecker::evalFreadFwrite, _1, _2, _3, _4,
+                  ErrorFEof | ErrorFError),
----------------
Szelethus wrote:
> What does this mean? I guess not the possible states after an fread call, but rather the possible states after a **failed** fread call, but then...
Name of the parameter may be misleading, for `evalFreadFwrite` the last argument describes the new state after the operation has failed.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:225-228
       {{"feof", 1},
        {&StreamChecker::preDefault,
-        &StreamChecker::evalFeofFerror<StreamState::FEof>,
-        0,
-        {StreamState::FError, StreamState::NoError}}},
-      // Note: ferror can result in Unknown if at the call there is a
-      // PossibleErrors with all 3 error states (including NoError).
-      // Then if ferror is false the remaining error could be FEof or NoError.
+        std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFEof),
+        0}},
----------------
Szelethus wrote:
> ...this doesn't make much sense, `feof` doesn't **cause** and error, it merely detects it.
At `evalFeofFerror` the last parameter simply indicates if it is the `feof` or the `ferror` case.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374