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

New registry shape #4815

Merged
merged 4 commits into from
Apr 17, 2024
Merged

New registry shape #4815

merged 4 commits into from
Apr 17, 2024

Conversation

robertbastian
Copy link
Member

@robertbastian robertbastian commented Apr 16, 2024

@robertbastian robertbastian marked this pull request as ready for review April 16, 2024 21:15
@robertbastian robertbastian requested review from Manishearth, sffc and a team as code owners April 16, 2024 21:15
sffc
sffc previously approved these changes Apr 17, 2024
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Praise: this is great! Glad it works

// This is a bit naughty, we need the marker's type, but we're actually
// baking its value. This works as long as all markers are unit structs.
Copy link
Member

Choose a reason for hiding this comment

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

Aha, I was thinking whether we should make all our marker types be empty enums in v2, but here's a use case where we need them as a value

Copy link
Member Author

@robertbastian robertbastian Apr 17, 2024

Choose a reason for hiding this comment

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

I'm happy to redesign this, I don't like how we currently do it. The #[data_struct] macro could add a non-self fn bake_type() -> TokenStream to the marker.

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

LG after diffbase

@robertbastian robertbastian merged commit 8c30d4a into unicode-org:main Apr 17, 2024
30 checks passed
@sffc
Copy link
Member

sffc commented Apr 17, 2024

Thought: The bake macro could generate a registry macro of the same form but containing only the keys in that one crate, and then the same cb could be used and given to different registry macros.

Of course what I want to use the registry! for is enabling easier creation of forking DataProvider impls. I was thinking that a way to do this would be if the registry created impls for all keys except a specific list. What I'm thinking is for the macro to generate code such as

#[if("MarkerA" != "MarkerB")]
#[if("MarkerA" != "MarkerC")]
impl DataProvider<MarkerA> for MyForkingProvider { ... }

#[if("MarkerB" != "MarkerB")]
#[if("MarkerB" != "MarkerC")]
impl DataProvider<MarkerB> for MyForkingProvider { ... }

#[if("MarkerC" != "MarkerB")]
#[if("MarkerC" != "MarkerC")]
impl DataProvider<MarkerC> for MyForkingProvider { ... }

#[if("MarkerD" != "MarkerB")]
#[if("MarkerD" != "MarkerC")]
impl DataProvider<MarkerD> for MyForkingProvider { ... }

With that generated code, impl DataProvider<MarkerB> and impl DataProvider<MarkerC> would be dropped, allowing them to be implemented ("specialized") in a custom way.

However, it's not immediately clear to me how to actually do this.

We could probably make a custom proc macro to help, if one doesn't already exist.

@robertbastian robertbastian deleted the reg branch April 17, 2024 15:31
@sffc
Copy link
Member

sffc commented Apr 17, 2024

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