Skip to content

Conversation

@powerboat9
Copy link
Collaborator

This patch moves node id storage/handling into a subclass, HasNodeId, and requires that nodes be explicitly assigned a node id before they can have their node id fetched.

Pros:

  • Removes the need for a distinction between copying with/out preserving node id
  • I'm pretty sure this is closer to what rustc does

Cons:

  • Node ids have to be explicitly assigned -- NodeIdVisitor is created to handle this
  • Nodes have to be copied before being assigned a node id, if the copy is to have a different node id -- a method for reassigning node ids could be added, but this seems like best practice anyways
  • HasNodeId needs to be explicitly initialized in copy constructors for derived classes. Otherwise the result of a copy has no assigned node id, instead of copying the original node's node id

Despite the cons, this is probably the best method available for solving the how-do-we-copy-nodes-with-or-without-node-id problem, as the alternative seems to be playing wack-a-mole with copy-vs-reconstruct in the derive handling code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant