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

[Csaba Dabis via Phabricator via cfe-commits]

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