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

Reworking attributes to match Ctx #474

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

UnsignedByte
Copy link
Collaborator

@UnsignedByte UnsignedByte commented Oct 28, 2024

This reworks attributes to use a system like the Ctx trait for attribute types. This would allow us to use a single get call with any type of attribute. This system currently has a few issues:

  • The Attr struct is currently used to parse attributes, but is pretty useless otherwise.
  • Ideally, we would like to be able to give different sets of attributes for different attribute contexts (ports vs components).

@UnsignedByte
Copy link
Collaborator Author

@rachitnigam Currently attribute types are specified in the utils directory but that doesn't seem right. I could also move them to ast instead if that seems more reasonable but they are also used in IR...

@rachitnigam
Copy link
Member

I don't have strong opinions on where they should live! The utils separation is probably because they are included by both ast and ir but we don't want to include ir in ast.

@UnsignedByte
Copy link
Collaborator Author

Yup, definitely agree with not including ir in AST. My main worry is that although the attribute traits and stuff could go into utils, the actual attributes themselves are somewhat tied to Filament and my impression of utils was that they should be "language-agnostic" compiler utilities... Maybe I will move attribs to AST.

@rachitnigam
Copy link
Member

No, the utils are utilities for the Filament compiler infrastructure. I don't think it is specifically architected to contained truly agnostic utilities.

@UnsignedByte
Copy link
Collaborator Author

Macro-ized a bunch of stuff but I'm holding off on adding port attributes for another pass.

@@ -57,7 +58,7 @@ pub struct Component {

// ============== Component signature ===============
/// Attributes of the component
pub attrs: ast::Attributes,
pub attrs: utils::comp_attrs::Attrs,
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to expose the comp_attrs submodule?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Technically each set of Attrs comes with three types: Bool, Num, and Attrs for boolean and numeric flags as well as a type alias for the struct with these two types of flags.

We could publicize the underlying types instead, but that would then look something like CompAttrs, CompBool, CompNum and PortAttrs, PortBool, PortNum (for port attributes). Matter of preference honestly, I don't really mind either way.

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.

2 participants