[PATCH] D103434: [analyzer] Allow visitors to run callbacks on completion

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:95
+  /// This method is run once the \ref Stop method is called.
+  virtual void OnStop(const ExplodedNode *Curr, BugReporterContext &BRC,
+                      PathSensitiveBugReport &BR) const = 0;
----------------
xazax.hun wrote:
> Are we anticipating visitors that do something else other than calling a callback when they are stopped? If that is not the case, I wonder if storing/invoking the visitor in this class would make more sense. 
I think it was decided in the last meet that we should not make this new abstraction overtly  //specific//. So a stoppable visitor is free to have a callback (like the `ReturnVisitor` now does) and is also free to have some more behavior in the `OnStop` method. Also making the `OnStop` method pure-virtual means that the sub-classes are required to override it (so it can't be ignored).


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:97
+                      PathSensitiveBugReport &BR) const = 0;
+  bool Stopped;
+
----------------
xazax.hun wrote:
> Should we allow derived classes or even others query if a visitor was stopped? Or is this only for catching bugs? 
Right now it can't be queried. The purpose of the `Stopped` field is primarily for catching bugs. (this ensures visitors are stopped exactly //once//).


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:104
+  /// Call this method once the visitor runs out of useful nodes to process.
+  void Stop(const ExplodedNode *Curr, BugReporterContext &BRC,
+            PathSensitiveBugReport &BR);
----------------
xazax.hun wrote:
> Do you anticipate cases where the visitor itself does not know that it needs to stop (e.g. in the Visit functions) so someone needs to call this from the outside?
No, I don't. Are you suggesting I should make it a protected method?


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:895
 
+using VisitorCallback = llvm::function_ref<void(
+    const ExplodedNode *, BugReporterContext &, PathSensitiveBugReport &)>;
----------------
vsavchenko wrote:
> `function_ref` is a reference, it doesn't own the function. It means that it shouldn't outlive the actual function object (very similar to `StringRef`).
So I guess this means that the callback must be a method/lambda that //persists// (ie, it can't be a lambda in a `VisitNode` method, but must be stored a field in the class).


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1156
+    // visitor.
+    Stop(N, BRC, BR);
+
----------------
I am **not** entirely certain this is the right place to call the Stop method. It seems reasonable but I am not sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103434