introduce a dedicated AST type - #156
Closed
zerbina wants to merge 1 commit into
Closed
Conversation
* `PackedTree` becomes a standalone type, only storing the node buffer * the literal-data storage `Literals` is moved into its own module * the new type `Ast` combines a packed tree with a literal-data storage
Collaborator
Author
|
This refactor will be made obsolete with the eventual arrival of #179. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The goal is to separate the generic node storage and traversal operations from the specific concept of an abstract syntax tree (=AST).
An AST requires some extra bits of information (e.g., metadata), but adding this to
PackedTreewould make the type it no longer applicable for use cases that want just a sequence of nodes and the associated operations without anything else.While there currently are no such use cases, I can foresee that changing in the future, so splitting the two things makes sense.
As is, the current code is a dead end. Conceptually, the node tree operations (e.g.:
child,next, etc.) do not care about the concrete storage type -- they only require an input type that's a random-access supporting container type whose element is a type with a mutablekindandvalproperty, nothing more. This makes them an ideal candidate for static interfaces (i.e.,concept), but said NimSkull feature is not yet stable enough.I've tried to approximate the ideal solution (static interfaces) by making
PackedTreetype only represent the node storage and have it be parameterized with the node type, withAstthen embedding aPackedTreeinstance and providing a wrapper template for everyPackedTreeroutine such thatAstcan be used in the same way aPackedTreecan be, but this is annoying to maintain and it also significantly hurts code clarity, since various routines forBuilder,ChangeSet, etc. have to use unnecessarily broad interfaces.Using inheritance would work a little better in practice, but is not really correct (an
Astcannot be used everywhere aPackedTreecan be, for example, serialization), and it would likely also surface some NimSkull bugs/issues with argument subtyping anditerators.Progress on this PR needs to wait until NimSkull supports working static interfaces. In the meantime,
PackedTreewill have to become theAsttype.