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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
