[PATCH] D104925: [Analyzer] Improve report of file read at end-of-file condition.

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

Szelethus added a comment.

The bug reports speak for themselves, they are awesome. Nice work!



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:28-30
+//===----------------------------------------------------------------------===//
+// Definition of state data structures.
+//===----------------------------------------------------------------------===//
----------------
Thank you! Big fan of these.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:34-35
 
+const char *Desc_StreamEof = "Stream already in EOF";
+const char *Desc_ResourceLeak = "Resource leak";
+
----------------
You don't seem to reuse these in this or the followup patch? Its not a big deal, I don't mind you creating them, just feels a bit odd to me.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:409
+                                        SymbolRef StreamSym) const {
+    return C.getNoteTag([StreamSym](PathSensitiveBugReport &BR) {
+      if (!BR.isInteresting(StreamSym) ||
----------------
Can you not capture `this` as well? We could eliminate `StreamChecker::Instance` that way I suppose.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:715-716
   StateFailed = StateFailed->set<StreamMap>(StreamSym, NewState);
-  C.addTransition(StateFailed);
+  if (IsFread && SS->ErrorState != ErrorFEof)
+    C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
+  else
----------------
Oh, this took me quite a bit. Can we change `SS` to something that reflects that its the current error state, not the new one? Like `CurrSS`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104925