[PATCH] D80015: [Analyzer][StreamChecker] Added support for 'fread' and 'fwrite'.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I think the warning about EOF could be a separate patch, and it could be implemented for most stream operations. The patch in large looks great, but I'm just not sure why we handle fwrite so differently. Could you please explain?



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:56
+  bool operator!=(const StreamErrorState &ES) const {
+    return NoError != ES.NoError || FEof != ES.FEof || FError != ES.FError;
+  }
----------------
How about `return !(*this == ES)`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:456-463
+  SymbolRef Sym = StreamVal.getAsSymbol();
+  if (Sym && State->get<StreamMap>(Sym)) {
+    const StreamState *SS = State->get<StreamMap>(Sym);
+    if (SS->ErrorState & ErrorFEof)
+      reportFEofWarning(C, State);
+  } else {
+    C.addTransition(State);
----------------
Do we not want this for `fwrite` as well?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:495
+    return;
+  Optional<NonLoc> CountVal = Call.getArgSVal(2).getAs<NonLoc>();
+  if (!CountVal)
----------------
`// The standard refers to this parameter as "nmemb", but almost everywhere else its called "count".`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:523
+  // FEOF).
+  if (!IsFread || (SS->ErrorState != ErrorFEof)) {
+    ProgramStateRef StateNotFailed =
----------------
Aaaah okay it took me a while. I think `SS->ErrorState != ErrorFEof` isn't as talkative as `isConstrained.*`, could you add just a line like "if we **know** the state to be EOF..."?

On another note, why is fwrite so special in this context?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80015