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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to