balazske marked 4 inline comments as done.
balazske added a comment.
The difference of fread and fwrite comes from the fact that `fwrite` can always
succeed, `fread` does not succeed in EOF state.
The plan is to add the file functions sequentially. From the currently
implemented functions only `fread` needs the warning in EOF state. When any
later function is added that fails in EOF state, the warning should be added
for that function at that time.
================
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;
+ }
----------------
Szelethus wrote:
> How about `return !(*this == ES)`?
Probably better (does not look better but is more bug-safe).
================
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);
----------------
Szelethus wrote:
> Do we not want this for `fwrite` as well?
It should be no problem to write into a file at the end of the file, the file
is extended.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:495
+ return;
+ Optional<NonLoc> CountVal = Call.getArgSVal(2).getAs<NonLoc>();
+ if (!CountVal)
----------------
Szelethus wrote:
> `// The standard refers to this parameter as "nmemb", but almost everywhere
> else its called "count".`
I looked up the functions in [[ https://en.cppreference.com/w/c/io/fread |
cppreference.com ]], there it is called `count`. But probably it is better to
use the standard name here?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:523
+ // FEOF).
+ if (!IsFread || (SS->ErrorState != ErrorFEof)) {
+ ProgramStateRef StateNotFailed =
----------------
Szelethus wrote:
> 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?
If it is an `fwrite`, the function call can always succeed (even after a
previously EOF or error).
If it is an `fread`, it can succeed but not after an exactly-EOF state.
A success transition is not needed in an `fread` with exactly EOF condition.
(If we have fread with non-exactly EOF, for example `ErrorFEof` and
`ErrorFError`, the success is needed because the read in `ErrorFError` can
succeed.)
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
