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

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

Szelethus added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:236-242
+  const BugMessageMap BugMessages = {
+      {&BT_FileNull, "Assuming opening the stream fails here"},
+      {&BT_UseAfterClose, "Stream closed here"},
+      {&BT_UseAfterOpenFailed, "Assuming opening the stream fails here"},
+      {&BT_IndeterminatePosition, "Assuming this stream operation fails"},
+      {&BT_StreamEof, "Assuming stream reaches end-of-file here"},
+      {&BT_ResourceLeak, "Stream opened here"}};
----------------
Well this looks odd: for both `BT_StreamEof` AND `BT_IndeterminatePosition` we want to display a `NoteTag` for a failed `fseek`, for one you print "Assuming stream reaches end-of-file here", for the other, "Assuming this stream operation fails". This seems to contradict your comment in the fseek evaluating function:

```
If fseek failed, assume that the file position becomes indeterminate in any case.
```

Also, these `BugTypes` should be responsible for the *error message*, not the `NoteTag` message. I'd prefer if we mapped an enum to these strings (`NoteTagMsgKind`?), pass that as well to `constructNoteTag`, and allow the caller to decide that for which `BugTypes` the `NoteTag` is worth displaying for.

I think such a 4-argument `constructNoteTag` would capture best what we want here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:387-391
+  /// Create a NoteTag to display a note if a later bug report is generated.
+  /// The `BT` should contain all bug types that could be caused by the
+  /// operation at this location.
+  /// If later on the path a problematic event (reported bug) happens with the
+  /// same type, the last place is found with a note tag with that bug type.
----------------
How about:

Create a `NoteTag` describing an stream operation (whether stream opening succeeds or fails, stream reaches EOF, etc).
As not all operations are interesting for all types of stream bugs (the stream being at an indeterminate file position is irrelevant to whether it leaks or not), callers can specify in `BT` for which `BugType`s should this note be displayed for.
Only the `NoteTag` closest to the error location will be added to the bug report.


================
Comment at: clang/test/Analysis/stream-note.c:51
+void check_note_fopen_fail() {
+  FILE *F = fopen("file", "r"); // expected-note {{Assuming opening the stream fails here}} expected-note {{Assuming pointer value is null}} expected-note {{'F' initialized here}}
+  fclose(F);                    // expected-warning {{Stream pointer might be NULL}}
----------------
I'd prefer an individual line for these `expected-.*` directives. Its down to personal preference, but I find that far easier to read.


================
Comment at: clang/test/Analysis/stream-note.c:36-41
-  FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+  FILE *F = fopen("file", "r");
   if (!F)
     // expected-note at -1 {{'F' is non-null}}
     // expected-note at -2 {{Taking false branch}}
     return;
-  F = freopen(0, "w", F); // expected-note {{Stream reopened here}}
----------------
Szelethus wrote:
> I think I preferred this, honestly.
Hmmm... I've given this some thought, and yes, I the stream misuse can indeed be captured starting from the last `freopen` call. The specialized message for reopen was nice, but I guess no actual information was lost by this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106262