Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unique ptr #17

Merged
merged 14 commits into from
Jul 19, 2019
Merged

Unique ptr #17

merged 14 commits into from
Jul 19, 2019

Conversation

leonardt
Copy link
Owner

Lots of code changes, I don't know how necessary it is to review it in great detail (assuming you trust the test suite), but the main change is to move all the AST nodes to use unique_ptr for their children. This enforces that all AST nodes have unique children, simplifying the memory management model (leveraging unique_ptr to clean things up). This prevents users from sharing a single instance of an AST node amongst multiple children. While this approach might exhibit better space consumption, it's likely to lead to problems (e.g. deleting a node when it's being used by someone else). If we think that's a valid use case, we could refactor this to use a shared_ptr model, but I think for now this is simplest and forces users to write code that will likely be easier to maintain (the user can always assume that a node owns all its children).

@rdaly525
Copy link
Collaborator

If everything is a unique ptr, how do we do something like:

module();
  reg a;
  reg b;
  always @(*) begin
    b = a + a;
  end 
endmodule

where an expression is referenced twice?

@rsetaluri
Copy link
Collaborator

@rdaly525 This is just an IR for the AST. So a + a would look like

BinOp(ADD, Identifier("a"), Identifier("a")).

Is this correct @leonardt?

@leonardt
Copy link
Owner Author

@rsetaluri is correct, the goal here is to enforce a simple and easy to understand memory model (at a cost to performance). My hunch is that most code related to this will not be bound memory consumption, rather it will be beneficial to have unique_ptr help us ensure that manipulations to the AST happen correctly. For example, if I want to replace a node, I know that all its children can be deleted because it won't be used by anyone else.

@leonardt leonardt merged commit 7ca3694 into master Jul 19, 2019
@leonardt leonardt deleted the unique-ptr branch July 19, 2019 22:49
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.

None yet

3 participants