Skip to content

Further reduce interned type boilerplate #554

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

Merged
merged 7 commits into from
Jul 4, 2020

Conversation

detrumi
Copy link
Member

@detrumi detrumi commented Jul 3, 2020

Builds on top of #550, using a macro to generate the sequence structs, interned methods and Sequence impls.

@detrumi detrumi force-pushed the interned-seq-refactor-macros branch from 86739e5 to 9362e9f Compare July 3, 2020 09:47
@jackh726
Copy link
Member

jackh726 commented Jul 3, 2020

This is more of what I was expecting. But if we do this, it's easy enough to just generate the Sequence functions in the macro.

I guess this is a fundamental tradeoff: would we rather have more "code" or use traits to share "code". The trait route would result in less code generation, but requires we bring the trait into scope.

As a side point, I don't really like Sequence, I would instead prefer something like InternedSlice

Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

This LGTM. But I would probably like a review from @nathanwhit and/or @nikomatsakis, since they seemed like they liked the trait approach. But your discretion @detrumi

Copy link
Member

@nathanwhit nathanwhit left a comment

Choose a reason for hiding this comment

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

I'm fine with this change 👍 . The main downside is that macros arguably make the code more opaque, but this is a pretty simple macro and definitely does have some ergonomic wins.

@detrumi detrumi merged commit 5cc3d08 into rust-lang:master Jul 4, 2020
@detrumi detrumi deleted the interned-seq-refactor-macros branch July 4, 2020 18:48
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.

3 participants