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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

A few nits inline, otherwise the patch is awesome, thank you!!



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:60-61
 private:
-  void Fopen(CheckerContext &C, const CallExpr *CE) const;
-  void Tmpfile(CheckerContext &C, const CallExpr *CE) const;
-  void Fclose(CheckerContext &C, const CallExpr *CE) const;
-  void Fread(CheckerContext &C, const CallExpr *CE) const;
-  void Fwrite(CheckerContext &C, const CallExpr *CE) const;
-  void Fseek(CheckerContext &C, const CallExpr *CE) const;
-  void Ftell(CheckerContext &C, const CallExpr *CE) const;
-  void Rewind(CheckerContext &C, const CallExpr *CE) const;
-  void Fgetpos(CheckerContext &C, const CallExpr *CE) const;
-  void Fsetpos(CheckerContext &C, const CallExpr *CE) const;
-  void Clearerr(CheckerContext &C, const CallExpr *CE) const;
-  void Feof(CheckerContext &C, const CallExpr *CE) const;
-  void Ferror(CheckerContext &C, const CallExpr *CE) const;
-  void Fileno(CheckerContext &C, const CallExpr *CE) const;
-
-  void OpenFileAux(CheckerContext &C, const CallExpr *CE) const;
-
-  ProgramStateRef CheckNullStream(SVal SV, ProgramStateRef state,
-                                 CheckerContext &C) const;
-  ProgramStateRef CheckDoubleClose(const CallExpr *CE, ProgramStateRef state,
-                                 CheckerContext &C) const;
+  typedef void (StreamChecker::*FnCheck)(const CallEvent &,
+                                         CheckerContext &) const;
+
----------------
Prefer using. When I wrote D68165, I spent about 10 minutes figuring out how to do it... Ah, the joys of the C++ syntax.
```lang=c++
using FnCheck = void (StreamChecker::*)(const CallEvent &,
                                        CheckerContext &) const;
```


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:109-110
 bool StreamChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
+  if (!Call.isGlobalCFunction())
+    return false;
+
----------------
Isn't this redundant with my other inline about parameter types?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:127-131
+  for (auto P : Call.parameters()) {
+    QualType T = P->getType();
+    if (!T->isIntegralOrEnumerationType() && !T->isPointerType())
+      return nullptr;
+  }
----------------
I'm not sure why we need this, is it true that *all* stream related functions return a pointer or a numerical value? Are we actually checking whether this really is a library function? If so, this looks pretty arbitrary.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:156
+void StreamChecker::evalFread(const CallEvent &Call, CheckerContext &C) const {
+  (void)CheckNullStream(Call.getArgSVal(3), C);
 }
----------------
Why the need for `(void)`? `CheckNullSteam` doesn't seem to have an `LLVM_NODISCARD` attribute.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67706/new/

https://reviews.llvm.org/D67706