baloghadamsoftware added inline comments.
================ Comment at: clang/test/Analysis/stream.c:188 + C = fgetc(fp); + fread(0, 0, 0, fp); // expected-warning {{Should call}} + fread(0, 0, 0, fp); ---------------- Szelethus wrote: > balazske wrote: > > Szelethus wrote: > > > Are we sure that this is the error message we want to emit here? Sure, > > > not checking whether the stream is in an error state is a recipe for > > > disaster, but this seems to clash with > > > > Should call 'feof' and 'ferror' after failed character read. > > > as error here is not even checking the return value. Shouldn't we say > > > > Should check the return value of `fgetc` before ... > > > instead? > > No, it is not enough to check the return value of `fgetc`, exactly this is > > the reason for the checker. "Should call 'feof' and 'ferror' after failed > > character read." is more exact: The read was suspected to fail because EOF > > was returned but should call feof (...) to verify it. A better message can > > be: > > > > If 'fgetc' returns EOF call 'feof' and 'ferror' to check for error. > The compliant solution in the rule description has the following example: > > ```lang=c++ > > #include <stdio.h> > > void func(void) { > int c; > > do { > c = getchar(); > } while (c != EOF); > if (feof(stdin)) { > /* Handle end of file */ > } else if (ferror(stdin)) { > /* Handle file error */ > } else { > /* Received a character that resembles EOF; handle error */ > } > } > ``` > because if the character resembles `EOF` that is an error too, and also > > > Calling both functions on each iteration of a loop adds significant > > overhead, so a good strategy is to temporarily trust EOF and WEOF within > > the loop but verify them with feof() and ferror() following the loop. > > Which makes me thing that the error here is not checking against `EOF`, first > and foremost. > > The warning message > > >If 'fgetc' returns EOF call 'feof' and 'ferror' to check for error. > > sounds great, I would just prefer to see it //after// we **know** that the > stream returned `EOF`. I agree. The checker should emit a warning if and only if the return value of `fgetc()` equals to `EOF`. Another checker should ensure that the return value of `fgetc()` is indeed compared to `EOF`. However: are we sure that there are no cases were no other error may happen than //EOF// thus we intentionally omit further checking? (Imagine a simple read from the keyboard in a command line utility.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75045/new/ https://reviews.llvm.org/D75045 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits