balazske marked 4 inline comments as done.
balazske added a comment.
An improvement can be made if the check runs only when size of `char` and `int`
are equal. And it is possible to avoid the warning if `fgetc` is called after
`fgetc` (with no functions in between). (More complicated thing is to test for
loop construct and maybe not more efficient.)
The code can be improved somewhat but there are now too many special cases and
there is already a default eval function. Probably it is better to have every
"check" function take and return `ExplodedNode` instead of state.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:255-261
+ if (EOFv == 0) {
+ if (const llvm::Optional<int> OptInt =
+ tryExpandAsInteger("EOF", C.getPreprocessor()))
+ EOFv = *OptInt;
+ else
+ EOFv = -1;
+ }
----------------
Szelethus wrote:
> I'm 99% sure that the `Preprocessor` is retrievable in the checker registry
> function (`register*Checker`), and this code should be moved there, and `EOFv
> ` be made non-mutable.
I am not sure if this would be good. The `Preprocessor` is not available easily
and the "register" function is probably there to register a checker and do
nothing else. Probably the source code is not even known when it is called. And
how to pass the EOF value to the created checker in the register function (if
not a static variable is used)?
================
Comment at: clang/test/Analysis/stream.c:182
+
+void check_fgetc_error() {
+ FILE *fp = fopen("foo.c", "r");
----------------
martong wrote:
> Do you have tests for `getc` as well? Maybe we could have at least one for
> getc too.
`getc` is not done yet. It can need more change in `StreamChecker` to handle
functions on std streams (at least stdin).
================
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:
> 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.
================
Comment at: clang/test/Analysis/stream.c:246-247
+ if (C == EOF) {
+ feof(fp);
+ ferror(fp);
+ }
----------------
Szelethus wrote:
> I bet this isn't how we envision how these function to be used. Maybe
> `ReturnValueChecker` could be deployed here? In any case, that would be a
> topic of a different revision.
If `fgetc` returns non-EOF we can be sure that there is no error, so no need to
call `ferror` and `feof`. If it returns EOF we must "double-check" that it was
a real error or an EOF-looking character that was read.
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
