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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> Tests to verify that ReturnVisitor actually does what we intend it to do (call the callback at the right place).

You mean write a test that demonstrates that? I guess unless we're willing to wait for the checker to catch up, a good approach to this would be to write a unit test. See `unittests/StaticAnalyzer` - there aren't many of them because they're relatively hard to write but these days they're getting more and more popular as we're trying to eliminate mutually exclusive bugs invisible on LIT tests. You can mock some code, emit a warning against it from a mocked checker, and test directly that the visitor is stopped at, say, a given node or a given line number.

Or you mean like, add an assert that ensures that you're doing right thing? I think this is also possible to some extent. For example, you can check when the visitor is stopped that no other visitors were added during the current `VisitNode()` invocation (otherwise the process has to continue). Or you could assert the opposite: if the visitor has run to completion and no other visitors were added then the callback has to be invoked. I think this can get messy really quickly (esp. knowing that you can't simply count all visitors to see if more were added, given how newly added visitors may be duplicates of existing visitors). But I suspect that it could be easy to check this in at least one direction.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:109
+// Callback type for trackExpressionValue
+using VisitorCallback = llvm::function_ref<void(
+    const ExplodedNode *, BugReporterContext &, PathSensitiveBugReport &)>;
----------------
I think we have to use `std::function` here.
>   lang=c++
>   /// An efficient, type-erasing, non-owning reference to a callable. This is
>   /// intended for use as the type of a function parameter that is not used
>   /// after the function in question returns.
>   ///
>   /// This class does not own the callable, so it is not in general safe to store
>   /// a function_ref.
>   template<typename Fn> class function_ref;

An `llvm::function_ref` becomes a dangling reference as soon as `trackExpressionValue()` returns. This is too early.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103434