Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land.
This patch is great. LGTM! In D80015#2043263 <https://reviews.llvm.org/D80015#2043263>, @balazske wrote: > If the unit of the change is adding `fread` and `fwrite` completely, the > warning with FEOF at `fread` is correct to add because it belongs to `fread`. > I was planning to add some file handling functions to the checker that have a > basic functionality implemented. In this case, it is the fread-fwrite > functions, but the "indeterminate file position" is still missing here (that > is a bigger change), so it is the "basic functionality" that is added. The > FEOF warning is probably too small to add it in a separate patch, also it is > added only to one place now. Very well, I'm sold. I didn't check that we don't really model another function that needs it. Woops :^) > There is intentionally no warning about ferror state, first reason is that > often ferror and "indeterminate file position" comes together and then the > warning will be produced for "indeterminate position" in the next change. > Next reason is that ferror in itself is not always cause for warning and we > can currently not decide about it. There was the example about write into a > file that is open in read mode that results in a ferror state, then the next > read should work (with ferror state at start). Another reason, the program > should still always handle if a file operation fails. If an operation starts > in ferror state, it will probably fail again but that error should be handled > by the application. > > Intent of these warnings is not to check that the error case was handled by > the user code, that is a more difficult thing. We could make a new warning > that warns if between two possible failing operations no `ferror` or `feof` > was called, this would check if the user handled the error. But not accurate > because the error can be handled based on the return value of the failed > function, without calling `ferror` or `feof` (except some special cases like > the `fgetc`). The only safe warning is to add if a function is called in > really unuseful way, for example fread in already FEOF state that do never > succeed, and the "indeterminate file position" case. The question is what is a good practice on ferror -- a check on `errno` and then a `clearerr`? In any case, you are right, this another discussion for another time. > My goal with these changes was to have a check for the CERT FIO34-C case, not > for the complete `StreamChecker` that could contain many other features. So > probably not even all file handling functions will be added for now. Sure, shoot me down if you feel I'm trying to dictate the direction of your project! :) ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:523 + // FEOF). + if (!IsFread || (SS->ErrorState != ErrorFEof)) { + ProgramStateRef StateNotFailed = ---------------- balazske wrote: > 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.) > 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..."? Could you please add these few words before commiting? 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
