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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
