[PATCH] D106262: [clang][analyzer] Use generic note tag in alpha.unix.Stream .

Mon Sep 13 03:46:14 PDT 2021

Szelethus added a comment.

I like everything I see here so far! As soon as those debug functions are in, the patch should land!

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:236-240
+  /// At any stream operation that can cause (multiple type of) bugs, we can
+  /// determine the failure reason text by only knowing the bug type. The same
+  /// text is applicable at every operation that may cause that bug. This map
+  /// is used to lookup the message text in a note tag that is added at the
+  /// failing operation.
----------------
What this means might be obvious to us, because we are very familiar with this and similar checkers, but I'm sure its confusing for anybody else, even for seasoned analyzer developers. I'd prefer if comments like these were accompanied with examples. There are a few decent ones I think in some of the comments I left on this revision, feel free to copy one here.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:408-410
+  /// At a bug report the last operation in the path that has added this kind of
+  /// NoteTag is the one that caused the bug. It is enough to know the bug type
+  /// to determine the note tag text.
----------------
How about you explain this logic thoroughly in one comment (maybe above `BugMessages`), and replace these last 3 lines with "See the comments for `BugMessages`."?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106262