This patch introduces the concept of *Stoppable* visitors, ie, a
visitor that must perform some task when it runs out of nodes on which
it has work to do.
Initially, the ReturnVisitor is being made *stoppable*, thus allowing
it to run callbacks on completion. These callbacks are meant to be
supplied from trackExpressionValue.
Thus, this patch will also allow trackExpressionValue to recieve a
callback to run when it is done tracking.
Details
- Reviewers
NoQ vsavchenko xazax.hun teemperor
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Note: This patch is only partially done.
The following things are still left:
- trackExpressionValue must receive a callback
- The callback must be passed to a visitor (ReturnVisitor for now)
- Tests to verify that ReturnVisitor actually does what we intend it to do (call the callback at the right place).
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp | ||
---|---|---|
895 | 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). |
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h | ||
---|---|---|
95 | 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. | |
97 | Should we allow derived classes or even others query if a visitor was stopped? Or is this only for catching bugs? | |
101 | I prefer really small functions, like this dtor to be in header files. Feel free to ignore. | |
104 | 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? |
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h | ||
---|---|---|
95 | 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). | |
97 | 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). | |
104 | No, I don't. Are you suggesting I should make it a protected method? | |
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp | ||
895 | 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). | |
1154 | I am not entirely certain this is the right place to call the Stop method. It seems reasonable but I am not sure. |
The following things are now done:
- trackExpressionValue must receive a callback
- The callback must be passed to a visitor (ReturnVisitor for now)
I still don't know a good way to test the following:
- Tests to verify that ReturnVisitor actually does what we intend it to do (call the callback at the right place).
@vsavchenko, @NoQ, @xazax.hun, @teemperor can you give some suggestions?
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.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h | ||
---|---|---|
109 | I think we have to use std::function here.
An llvm::function_ref becomes a dangling reference as soon as trackExpressionValue() returns. This is too early. |
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h | ||
---|---|---|
95 | According to the conventions, should use stop and onStop instead of Stop, OnStop? |
I was thinking a lot about this problem after our last call, and even though StoppableVisitor helps to solve the problem that we have, it is extremely unclear when this callback is called and should be called.
I decided to restore the balance (and rid of all younglings, maybe) and put together this interface: D103605. I still need to migrate the existing code into the new architecture, but it can be done incrementally.
I am closing this since it has been addressed much better by the patches from @vsavchenko.
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.