Charusso added a reviewer: Charusso.
Charusso requested changes to this revision.
Charusso added a comment.
This revision now requires changes to proceed.
Could you please mark resolved issues as resolved? I would like to see what we
miss, and what has been done.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:61
+ using FnCheck = bool (StreamChecker::*)(const CallEvent &,
+ CheckerContext &) const;
+
----------------
I prefer `std::function`, because it is modern.
```
using StreamCheck =
std::function<void(const StreamChecker *, const CallEvent &,
CheckerContext &)>;
```
I think it is fine with pointers, but at some point we need to modernize this.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:64
+ CallDescriptionMap<FnCheck> Callbacks = {
+ {{"fopen"}, &StreamChecker::evalFopen},
+ {{"tmpfile"}, &StreamChecker::evalTmpfile},
----------------
Because `evalFopen()` is basically the `OpenFileAux(Call, C);`, I think we
could simplify the API by removing unnecessary one-liners so here you could
write `{{"fopen"}, &StreamChecker::OpenFileAux`, that is why the `CallEvent`
parameter is so generic and useful.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:99
+ Optional<ProgramStateRef> CheckNullStream(SVal SV, CheckerContext &C) const;
+ Optional<ProgramStateRef> CheckDoubleClose(const CallEvent &Call,
+ CheckerContext &C) const;
----------------
This `Optional` is not necessary. When the state is changed, you can rely on
`CheckerContext::isDifferent()` to see whether the modeling was succeeded.
Therefore you could revert the `bool` return values as well.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:117
- ASTContext &Ctx = C.getASTContext();
- if (!II_fopen)
- II_fopen = &Ctx.Idents.get("fopen");
- if (!II_tmpfile)
- II_tmpfile = &Ctx.Idents.get("tmpfile");
- if (!II_fclose)
- II_fclose = &Ctx.Idents.get("fclose");
- if (!II_fread)
- II_fread = &Ctx.Idents.get("fread");
- if (!II_fwrite)
- II_fwrite = &Ctx.Idents.get("fwrite");
- if (!II_fseek)
- II_fseek = &Ctx.Idents.get("fseek");
- if (!II_ftell)
- II_ftell = &Ctx.Idents.get("ftell");
- if (!II_rewind)
- II_rewind = &Ctx.Idents.get("rewind");
- if (!II_fgetpos)
- II_fgetpos = &Ctx.Idents.get("fgetpos");
- if (!II_fsetpos)
- II_fsetpos = &Ctx.Idents.get("fsetpos");
- if (!II_clearerr)
- II_clearerr = &Ctx.Idents.get("clearerr");
- if (!II_feof)
- II_feof = &Ctx.Idents.get("feof");
- if (!II_ferror)
- II_ferror = &Ctx.Idents.get("ferror");
- if (!II_fileno)
- II_fileno = &Ctx.Idents.get("fileno");
-
- if (FD->getIdentifier() == II_fopen) {
- Fopen(C, CE);
- return true;
- }
- if (FD->getIdentifier() == II_tmpfile) {
- Tmpfile(C, CE);
- return true;
- }
- if (FD->getIdentifier() == II_fclose) {
- Fclose(C, CE);
- return true;
- }
- if (FD->getIdentifier() == II_fread) {
- Fread(C, CE);
- return true;
- }
- if (FD->getIdentifier() == II_fwrite) {
- Fwrite(C, CE);
- return true;
- }
- if (FD->getIdentifier() == II_fseek) {
- Fseek(C, CE);
- return true;
- }
- if (FD->getIdentifier() == II_ftell) {
- Ftell(C, CE);
- return true;
- }
- if (FD->getIdentifier() == II_rewind) {
- Rewind(C, CE);
- return true;
- }
- if (FD->getIdentifier() == II_fgetpos) {
- Fgetpos(C, CE);
- return true;
- }
- if (FD->getIdentifier() == II_fsetpos) {
- Fsetpos(C, CE);
- return true;
- }
- if (FD->getIdentifier() == II_clearerr) {
- Clearerr(C, CE);
- return true;
- }
- if (FD->getIdentifier() == II_feof) {
- Feof(C, CE);
- return true;
- }
- if (FD->getIdentifier() == II_ferror) {
- Ferror(C, CE);
- return true;
- }
- if (FD->getIdentifier() == II_fileno) {
- Fileno(C, CE);
- return true;
+ return (this->*Callback)(Call, C);
+}
----------------
Why do you inject `this`?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:137
+ return *Callback;
}
----------------
I would move this tiny `identifyCall()` into `evalCall()` as the purpose of
`evalCall()` is the validation of the `Callback` in this case.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67706/new/
https://reviews.llvm.org/D67706
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
