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

Add @RegisterBank macros #2

Merged
merged 3 commits into from
Sep 22, 2023
Merged

Add @RegisterBank macros #2

merged 3 commits into from
Sep 22, 2023

Conversation

rauhul
Copy link
Collaborator

@rauhul rauhul commented Sep 18, 2023

Adds a pair of macros used to define a bank of registers directly in Swift code called @RegisterBank and @RegisterBank(offset:). Referred to as bank and offset macro respectively.

The bank macro converts a struct defining the layout of nested registers and nested register banks into a reference type containing the base address of the structure. The offset macro converts stored properties into computed properties offset from the base address introduced by the bank macro. The bank and offset macros emit a number of diagnostics and fix-its to aid programmers.

Adds a variety of test cases for each macro individually and both used in conjunction.

Adds a pair of macros used to define a bank of registers directly in
Swift code called `@RegisterBank` and `@RegisterBank(offset:)`. Referred
to as bank and offset macro respectively.

The bank macro converts a struct defining the layout of nested registers
and nested register banks into a reference type containing the base
address of the structure. The offset macro converts stored properties
into computed properties offset from the base address introduced by the
bank macro. The bank and offset macros emit a number of diagnostics and
fix-its to aid programmers.

Adds a variety of test cases for each macro individually and both used
in conjunction.
@rauhul rauhul added the enhancement New feature or request label Sep 18, 2023
@rauhul rauhul self-assigned this Sep 18, 2023
Copy link
Contributor

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Looks good overall. I left a couple comments from skimming through the code.

Sources/MMIOMacros/RegisterBankOffsetMacro.swift Outdated Show resolved Hide resolved
Sources/MMIOMacros/RegisterBankOffsetMacro.swift Outdated Show resolved Hide resolved
Sources/MMIOMacros/RegisterBankOffsetMacro.swift Outdated Show resolved Hide resolved
Sources/MMIOMacros/RegisterBankOffsetMacro.swift Outdated Show resolved Hide resolved
Sources/MMIOMacros/RegisterBankOffsetMacro.swift Outdated Show resolved Hide resolved
Sources/MMIOMacros/RegisterBankOffsetMacro.swift Outdated Show resolved Hide resolved

import SwiftSyntax

protocol HasIntroducerKeyword {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would make sense as a trait in swift-syntax as well. Would you mind opening a PR for it or file an issue for it?

Copy link
Collaborator Author

@rauhul rauhul Sep 19, 2023

Choose a reason for hiding this comment

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

It's not clear to me how to represent this in the swift-syntax trait system since the mapping from typeKeyword to introducerKeyword must be manually specified?

Choose a reason for hiding this comment

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

It ought to be possible to protocol-ize it, I'd think. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Clearly the requirement is able to be factored into a protocol, what isn't obvious is how to generate the mapping from each conformer to the protocol's introducerKeyword requirement using swift-syntax's Trait DSL.

Copy link
Member

@phausler phausler left a comment

Choose a reason for hiding this comment

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

Overall the macros look pretty clean.

}

// Binding must have a type annotation.
guard let type = binding.typeAnnotation?.type else {
Copy link
Member

Choose a reason for hiding this comment

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

if we had some sort of #type(of:) that would emit a type at type resolution time would that alleviate this restriction? Observable ran into similar issues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So really I could drop a lot of these diagnostics. What I really want to assert is that the type of the stored property that the macro was attached to conforms to a particular protocol, but as you know thats not possible.

I added a bunch of diagnostics here to hopefully lead people in the right direction, but its not as fool proof as I would like.

Copy link
Member

Choose a reason for hiding this comment

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

it does make me wonder if this is perhaps a job for property wrappers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't reach for self's unsafeAddress from a wrapper. Property wrappers always own their storage. (there's an escape hatch for classes, but that doesn't apply here)

@@ -0,0 +1,14 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

probably don't want the resolved in here right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe, I'm not sure how I feel about omitting it

@rauhul rauhul merged commit c01aad4 into main Sep 22, 2023
@rauhul rauhul deleted the register-bank-macros branch September 22, 2023 17:07
rauhul added a commit that referenced this pull request Nov 17, 2023
Adds a pair of macros used to define a bank of registers directly in
Swift code called `@RegisterBank` and `@RegisterBank(offset:)`. Referred
to as bank and offset macro respectively.

The bank macro converts a struct defining the layout of nested registers
and nested register banks into a reference type containing the base
address of the structure. The offset macro converts stored properties
into computed properties offset from the base address introduced by the
bank macro. The bank and offset macros emit a number of diagnostics and
fix-its to aid programmers.

Adds a variety of test cases for each macro individually and both used
in conjunction.
rauhul added a commit that referenced this pull request Nov 17, 2023
Adds a pair of macros used to define a bank of registers directly in
Swift code called `@RegisterBank` and `@RegisterBank(offset:)`. Referred
to as bank and offset macro respectively.

The bank macro converts a struct defining the layout of nested registers
and nested register banks into a reference type containing the base
address of the structure. The offset macro converts stored properties
into computed properties offset from the base address introduced by the
bank macro. The bank and offset macros emit a number of diagnostics and
fix-its to aid programmers.

Adds a variety of test cases for each macro individually and both used
in conjunction.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants