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.

Sign up for GitHub

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

Jump to bottom

implement const generics #148

Merged
merged 2 commits into from
Oct 29, 2021
Merged

implement const generics #148

merged 2 commits into from
Oct 29, 2021

Conversation

Stebalien
Copy link
Member

Rebased and squashed from #116.

Closes #114.

@Stebalien
Copy link
Member Author

Motivation for moving this forward now:

  1. The UX concerns raised in the initial PR are likely to remain problems for the foreseeable future.
  2. Multiple downstream users have asked for this (and are forking the library to make it happen).
  3. The main downstream users (libp2p, forest).
  4. Some downstream libraries are unable to move to "generic" CIDs and need boxed CIDs. Const generics will allow for things like Unsized Cids argumentcomputer/sp-cid#5 to get boxed CIDs without paying for dynamic dispatch.

@Stebalien Stebalien requested a review from vmx October 29, 2021 01:26
vmx
vmx reviewed Oct 29, 2021
View reviewed changes
Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

Could you please add a BREAKING CHANGE section to the commit message, explaining the UX changes (and other breaking changes in case there are any), so that people that upgrade know what to do (eventually I'd like to generate a changelog from the commit messages, but I never found the time to do that).

@vmx
Copy link
Member

vmx commented Oct 29, 2021

@koushiro You are the only Scale Codec user I know of. Would you mind having a look if things still work as expected. Would you be up for adding tests for the Scale Codec in a separate PR? I always fear we break that part accidentally.

@mriise
Copy link
Contributor

mriise commented Oct 29, 2021

note for number 4, all the proposed ways of keeping the current API interface the same make use of storing the size in an associated constant which is NOT object safe.

mriise and others added 2 commits October 29, 2021 08:14
@mriise @Stebalien
implement const generics
1d3338a 
Rebased and squashed from #116. Closes #114.

BREAKING CHANGE: This change replaces uses of typenum with const
generics.

Importantly, the MultihashDigest, Hasher, and StatefulHasher traits are
now generic over the digest size due to compiler limitations. However,
`Hasher` exposes this size as an associated const, so
`MyHasherImpl::SIZE` can often be used to fill in the size parameter
with some help from type inference.

Additionally:

- All usages of typenum must be replaced with usize constants.
- The new MSRV is 1.51.0 due to const generics.
- IdentityDigest is now a tuple struct of usize and the digest instead
  of u8 and a generic array.
@Stebalien
make arbitrary generic over size
31ebd48
@Stebalien
Copy link
Member Author

note for number 4, all the proposed ways of keeping the current API interface the same make use of storing the size in an associated constant which is NOT object safe.

Multihashes/CIDs can be made object safe. But yeah, constructing a function that takes a Code and returns a boxed Multihash will likely still be tricky.

@Stebalien Stebalien changed the base branch from master to next October 29, 2021 15:30
@Stebalien
Copy link
Member Author

Merging to a next branch so we can batch up a bunch of breaking changes without endangering master.

@Stebalien Stebalien merged commit af6d069 into next Oct 29, 2021
@Stebalien Stebalien deleted the feat/const-generic branch October 29, 2021 15:36
@koushiro
Copy link
Contributor

koushiro commented Nov 1, 2021

@koushiro You are the only Scale Codec user I know of. Would you mind having a look if things still work as expected. Would you be up for adding tests for the Scale Codec in a separate PR? I always fear we break that part accidentally.

Sorry for the late reply, the scale-codec part LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vmx vmx vmx left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use const generics
4 participants
@Stebalien @vmx @mriise @koushiro