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@-1 {{'F' is non-null}}
// expected-note@-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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
