[PATCH] D75163: [analyzer][StreamChecker] Adding precall and refactoring.

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

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

LGTM! It makes much more sense to do the checking in `preCall` and the modeling in `evalCall`, the comments in the patch are precise and helpful, the tests cover everything. While I feel fairly confident that this patch is great both in concept and in execution, please leave some time for others to respond -- if that doesn't happen in a day or two, I think this is ready to land.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:64-70
+/// Get the value of the stream argument out of the passed call event.
+/// The call should contain a function that is described by Desc.
+SVal getStreamArg(const FnDescription *Desc, const CallEvent &Call) {
+  assert(Desc && Desc->StreamArgNo != ArgNone &&
+         "Try to get a non-existing stream argument.");
+  return Call.getArgSVal(Desc->StreamArgNo);
+}
----------------
You could make this a method of `FnDescription`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:119
+  /// If it can only be NULL a fatal error is emitted and nullptr returned.
+  /// Otherwise a new state where the stream is constrained to be non-null.
+  ProgramStateRef ensureStreamNonNull(SVal StreamVal, CheckerContext &C,
----------------
  /// If it can only be NULL a fatal error is emitted and nullptr returned.
  /// Otherwise the return value is a new state where the stream is constrained to be non-null.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:130
+
+  /// Check that the stream is in opened state.
+  /// If the stream is known to be not opened an error is generated
----------------
 /// Check that the stream is in an opened state.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:148-151
+    // Ensure that not a member function is called.
+    const auto *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
+    if (!FD || FD->getKind() != Decl::Function)
+      return nullptr;
----------------
I vaguely recall us having this conversation once, but isn't this redundant with 

```lang=c++
    if (!Call.isGlobalCFunction())
      return nullptr;
```
? I don't mind not addressing this before commiting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75163