[PATCH] D67706: [clang][analyzer] Using CallDescription in StreamChecker.

[Artem Dergachev via Phabricator via cfe-commits]

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