On Tue, Sep 8, 2020 at 11:29 AM Victor Khomenko <
victor.khomenko_at_[hidden]> wrote:

> > * If you wish to re-use the visitor, you need a mechanism to
> update
> > the captured reference, and you must remember to update it every time you
> > call the visitor, and there can be many call sites.
> >
> > In the code I showed, the visitor is stateless; it has no captures.
>
> In your example, the visitor is the lambda that captures variables "foo"
> and "visitor" by reference, not the variable "visitor", as you call
> std::visit on the lambda. I guess your point is that this lambda is cheap
> to create, so one can do it at every call.


Yes, that's right. Calls to std::visit usually involve a lambda already,
even if it's just
    std::visit([](auto&& x) { std::cout << x; }, v);
 or
    std::visit([&](auto&& x) { MyCustomVisit(x); }, v);


> Can you show an example?
>
> My AST examples are too long to be posted, so I'd rather make up an
> artificial one.


Artificial is fine, as long as
- it's compilable, and
- it demonstrates the problem you're concerned about.

Giving an example that doesn't clearly demonstrate the problem is just a
waste of time, because the response to the example will always be "Ah,
you're doing it wrong, here's how I'd do it instead." You want to pick an
example where you can *without additional round-trips* take the pattern
displayed in that response and map it back onto your actual codebase. I
hope — but am not confident! — that your new example clearly demonstrates
the problem you're asking about, and that therefore my solution will help
you.

  using ASTNode = variant<Constant, Variable, UnaryMinus, BinaryMinus, +
> 100 more>;
>
> // this visitor takes ASTNode&& and returns a vector of "useful"
> sub-trees
> struct CollectUsefulSubtreesVisitor{
> unordered_map<Blah> context; // large, so copying is expensive
> vector<ASTNode> useful; // accumulates "useful" subtrees
>

To be clear: ASTNode is a value type, and so the only way to accumulate
this vector of value-semantic subtrees is by *copying* subtrees out of the
original tree, or by *dismantling* the original tree, is that right?

If I wanted to refer to a specific subtree of a larger tree, I'd use a
(smart) pointer to the root of the subtree. In other words, I would expect
this member to be of type `vector<std::reference_wrapper<ASTNode>>`.


>
> // simplifies recursive calls
> void recurse(ASTNode&& n){
> special_visit(*this,move(n));
> }
>

At first glance, I don't understand why you're passing everything around by
rvalue reference.
At second glance, I assume that you're dismantling the given tree. This
also means that you expect never to be in a situation where you have two
interesting subexpressions S and T, and S is a subtree of T. For example,
in (x + (y*z)) it is never the case that *both* (y*z) and z are
interesting. Right?
Anyway, I'm still inclined to build up a vector of reference_wrappers (or
pointers) to pieces of the given original tree, and then allow the caller
to pilfer out of any-or-all of them after visitation is done. That just
seems simpler to reason about, compared to dismantling the tree on the fly.


     void operator()(Variable&& v, ASTNode&& n){
> // breaks unique ownership, even if one of the &&s is
> replaced by &
> useful.push_back(move(n)); // variables are always
> "useful"; make use of 2nd parameter
>

With the code as you've written it (vector of ASTNodes, not vector of
references to ASTNodes), this line might as well just use `v` directly:
    useful.push_back(std::move(v));
Remember that a variant is constructible from any of its alternative types,
so it's fine to construct an ASTNode from Variable `v`.

This also highlights the bug on your next line so well that even the
static-analyzer can see it:


> // fiddle with the context
> fiddle(context,v); // Ouch!!! moving n tacitly broke v!
>

Right. This breakage will happen regardless of whether you move(n) or
move(v). So you've got a logic bug here; no amount of physical refactoring
will solve the logic issue. You'll have to redo your algorithm so that
*semantically* the algorithm no longer needs to refer to a node after it's
been dismantled. I continue to suggest using a vector of
references/pointers to ASTNodes so that the original tree remains intact
for the entire visitation process.

HTH,
Arthur