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

Tagged union #97

Merged
merged 5 commits into from
Sep 24, 2019
Merged

Tagged union #97

merged 5 commits into from
Sep 24, 2019

Conversation

cdonovick
Copy link
Collaborator

@cdonovick cdonovick commented Sep 24, 2019

Adds TaggedUnion syntax as described in cdonovick/peak#68 (comment)

Breaks GarnettFlow because change to internal interfaces that magma will have to adopt but it should be a 1 line change in magma/product.py

Copy link
Collaborator

@rdaly525 rdaly525 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look good.

If I understand correctly, each ADT type is either "attribute based" and/or "getitem based". Each ADT metaclass can now inherit from one or both of these and have the fields accessed via attribute and/or getitem. In addition one can specify if each of these ADT types are ordered. This would probably make it easier to make a coproduct adt type (ordered Sum) or an unordered Product.

@rdaly525
Copy link
Collaborator

One minor issue, is that I am sure we have code scattered throughout that checks if a type is (Product, Tuple, Sum). These will have to be augmented to included tagged union. This should not block the PR though.

@cdonovick
Copy link
Collaborator Author

cdonovick commented Sep 24, 2019

@rdaly525

If I understand correctly, each ADT type is either "attribute based" and/or "getitem based". Each ADT metaclass can now inherit from one or both of these

Correct

and have the fields accessed via attribute and/or getitem

In practice yes but not really what's going on. AttrSyntax defines fields with attributes (x = int), it does not however mandate anything about the generated class. It just collects the fields and hands them off to _from_fields. GetitemSyntax defines fields with __getitem__ and again doesn't mandate anything about how the index should be interpreted. For instance AbstractBitvectorMeta could be potentially inherit from GetitemSyntax.

In addition one can specify if each of these ADT types are ordered. This would probably make it easier to make a coproduct adt type (ordered Sum) or an unordered Product

Correct
.

@phanrahan
Copy link
Collaborator

Looks good to me! Tests are clear.

@cdonovick cdonovick merged commit 58925b5 into master Sep 24, 2019
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