NoQ added a comment.
In D67706#1685963 <https://reviews.llvm.org/D67706#1685963>, @Szelethus wrote:
> It seems like this patch is diffed against your latest commit, not the master
> branch.
Yeah, seems so. The code looks great tho, thanks!
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:64
public:
- StreamChecker()
- : CD_fopen("fopen"), CD_tmpfile("tmpfile"), CD_fclose("fclose", 1),
- CD_fread("fread", 4), CD_fwrite("fwrite", 4), CD_fseek("fseek", 3),
- CD_ftell("ftell", 1), CD_rewind("rewind", 1), CD_fgetpos("fgetpos", 2),
- CD_fsetpos("fsetpos", 2), CD_clearerr("clearerr", 1),
- CD_feof("feof", 1), CD_ferror("ferror", 1), CD_fileno("fileno", 1) {}
+ StreamChecker() = default;
----------------
Feel free to omit the constructor entirely. We almost never have constructors
in our checkers.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:74-87
+ {{CDF_MaybeBuiltin, "fopen"}, &StreamChecker::evalFopen},
+ {{CDF_MaybeBuiltin, "tmpfile"}, &StreamChecker::evalTmpfile},
+ {{CDF_MaybeBuiltin, "fclose", 1}, &StreamChecker::evalFclose},
+ {{CDF_MaybeBuiltin, "fread", 4}, &StreamChecker::evalFread},
+ {{CDF_MaybeBuiltin, "fwrite", 4}, &StreamChecker::evalFwrite},
+ {{CDF_MaybeBuiltin, "fseek", 3}, &StreamChecker::evalFseek},
+ {{CDF_MaybeBuiltin, "ftell", 1}, &StreamChecker::evalFtell},
----------------
Are you sure these functions are actually sometimes implemented as builtins? I
think they're required to be regular functions.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:90-106
+ FnCheck identifyCall(const CallEvent &Call, CheckerContext &C,
+ const CallExpr *CE) const;
+
+ void evalFopen(CheckerContext &C, const CallExpr *CE) const;
+ void evalTmpfile(CheckerContext &C, const CallExpr *CE) const;
+ void evalFclose(CheckerContext &C, const CallExpr *CE) const;
+ void evalFread(CheckerContext &C, const CallExpr *CE) const;
----------------
For most purposes it's more convenient to pass `CallEvent` around. The only
moment when you actually need the `CallExpr` is when you're doing the binding
for the return value, which happens once, so it's fine to call
`.getOriginExpr()` directly at this point.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:125-127
const auto *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
if (!FD || FD->getKind() != Decl::Function)
return false;
----------------
(not your fault but before i forget) This check is actually redundant.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:273
+
+ if (SymbolRef Sym = RetVal.getAsSymbol()) {
+ // if RetVal is not NULL, set the symbol's state to Opened.
----------------
(not your fault but before i forget) This condition is actually always true
here.
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
