AST cloning mechanism #2699
AST cloning mechanism #2699
Conversation
This is intended to be used for template instantiation, but for now takes no stance as to what it's cloning.
github-actions
bot
added
the
explorer
| print("void CloneImpl(Arena& arena, CloneContext& context,") | ||
| print(" const AstNode& node, AstNode** result) {") | ||
| print(" switch(node.kind()) {") | ||
| for node in classes.values(): | ||
| if node.kind == Class.Kind.CONCRETE and node.Root().name == "AstNode": | ||
| print(f" case AstNodeKind::{node.name}:") | ||
| print( | ||
| f" return arena.New<{node.name}>(" | ||
| + "Arena::WriteAddressTo{result}, context, " | ||
| + f"static_cast<const {node.name}&>(node));" | ||
| ) | ||
| print(" }") | ||
| print("}\n") |
There was a problem hiding this comment.
I admit, reading this, I wonder if it'd be better to emit a .def file with an x-macro, and then have this be a source file. I think it'd avoid the ast_headers dependencies you're adding, and may be a little more readable as an end result. I'll leave that to you though if you want to make that change.
(I've been mulling if gen_rtti could be replaced with x-macros completely, which may be affecting my mindset)
There was a problem hiding this comment.
I'll look into this as a follow-up.
explorer/ast/clone_context.h
Outdated
| // Clone the given node, and remember the mapping from the original to the | ||
| // new node for remapping. | ||
| auto Clone(Nonnull<const AstNode*> node) -> Nonnull<AstNode*>; | ||
|
|
||
| // Clone the given value, replacing references to cloned local declarations | ||
| // with references to the copies. | ||
| auto Clone(Nonnull<const Value*> value) -> Nonnull<Value*>; | ||
|
|
||
| // Clone the given element reference. | ||
| auto Clone(Nonnull<const Element*> elem) -> Nonnull<Element*>; |
There was a problem hiding this comment.
Should these be private? I'm wondering, as a change, if you could do something like (requiring renaming these to ClonePointer):
template <typename T, typename = std::enable_if_t<
std::is_convertible_v<const T*, const AstNode*> ||
std::is_convertible_v<const T*, const Value*> ||
std::is_convertible_v<const T*, const Element*>>>
auto Clone(Nonnull<const T*> node) -> Nonnull<T*> {
return static_cast<T*>(ClonePointer(node));
}
or even:
template<typename T, typename = std::enable_if_t<
std::is_invocable_v<decltype(&CloneContext::ClonePointer), CloneContext&, const T*>>>
auto Clone(Nonnull<const T*> node) -> Nonnull<T*> {
return static_cast<T*>(ClonePointer(node));
}
(may have the syntax a little off)
The main benefit of this that I see is it feels like a simplification of the below Clone body. But I think with everything named Clone, that'd be self-recursive.
Note, that does slip away a little from your current approach of using overloads for everything, so I understand if you don't want it, but the way that you're currently casting pointers to their parent type to get the explicit overload you want seems like it increases the cost of understanding.
There was a problem hiding this comment.
Done something along these lines.
explorer/ast/clone_context.h
Outdated
| // that might refer to something being cloned or might refer to the original | ||
| // object. The returned node might not be fully constructed and should not be | ||
| // inspected. | ||
| auto Remap(Nonnull<AstNode*> node) -> Nonnull<AstNode*> { |
There was a problem hiding this comment.
Just to double-check, where is this used from? I'm wondering if it's necessary versus putting the implementation into Remap(Nonnull<T*> node)
There was a problem hiding this comment.
Folded the two functions together.
explorer/ast/clone_context.h
Outdated
| // Clone the given node if it's not already been cloned. This should be used | ||
| // very sparingly, in cases where ownership is unclear. | ||
| void MaybeClone(Nonnull<const AstNode*> node); |
There was a problem hiding this comment.
Is this used outside of the cpp file? Maybe CloneValueTransform could be a member class instead of separated (only forward declared in the header), so that this could be private instead of public, preventing external use?
zygoloid
force-pushed
the
explorer-clone
branch
from
4a1645e
to
45a8b1b
Compare
The strategy that we use for now to support template instantiation is to check the impl declaration as if it were a generic, but to defer all checking of the impl definition until we see a use in which all template parameters have arguments. At that point, we clone the impl definition and type-check the whole thing, with constant values set on the template parameters corresponding to the given arguments. No caching of template instantiations is performed yet; each time we form a reference to a template instantiation, we instantiate it afresh. We also don't implement the name lookup rule from #949 yet; lookups during template instantiation look only in the actual type and not in the constraint. Depends on #2699
This keeps all of our C++ code written in C++ source files, and avoids the increasing size of the RTTI generation script we'd started to see in #2699. Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
This is intended to be used for template instantiation, but for now takes no stance as to what it's cloning.
This is tested by parsing and cloning all of our test files, and checking that the result of converting each AST to proto is the same as the result of cloning and then converting each AST to proto.