-
Notifications
You must be signed in to change notification settings - Fork 59
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
const generics #116
Closed
Closed
const generics #116
Changes from 7 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
5c4cdcb
Revert "(cargo-release) version 0.11.3"
mriise 18f51a2
lazy move to const generics
mriise daa059e
remove std::usize
mriise ba1a6a9
use usize for maximum hash size instead of u8
mriise 2e193e5
fix: make derive macro work
vmx bfed660
fix sha macro
mriise 224dbe7
final changes & cargo clippy/fmt
mriise f040069
update proc macro messages and tests
mriise 0354a47
fix syntax and wordings
mriise 72108e6
Merge branch '0354a47'
mriise abd0fa6
Merge branch 'master' into master
mriise 616bc45
cargo fmt"
mriise 08f185f
Merge branch 'master' of https://github.com/mriise/rust-multihash
mriise 78bc734
make clippy happy, deny unsafe
mriise 415d110
rename SIZE to S
mriise a9d5c08
update parity codec
mriise 8889d86
Merge branch 'master' into master
mriise 5cea7ed
rewrite sha finalize()
mriise 65f3127
raise limit of varint to u16, use associated const
mriise 7adfd4b
derive copy for Multihash & Digest
mriise 9162fe3
revert max size
mriise 750f5be
dont update README in this pr"
mriise File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should still be an associated constant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's
Self::SIZE
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self::SIZE
replacedsize()
since while it could be aconst fn
it made more sense (in my mind) to make it a public associated constant.It would make sense to move back to
const fn size()
in the case of havingSIZE
be an associated constant without being repetitive.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems using an assosicated const as a paramater is still part of the incomplete
const_generics
feature in nightly, though with enough feature flags we can make it look something like thishttps://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=cfcf3ec66baa902500cae4dbcefd7dff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not being able to have it as associate constant in current stable Rust sucks :-/
I guess we leave it like that then, until it;s possible in stable Rust? What do others think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we in a hurry to merge this? it's a usability regression, and comparing it to the benefits, maybe we should hold off until more of const-generics is stabilized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not in a hurry. Though I fear that it will take a long time until it's usable on stable.
@mriise What do you think about perhaps fixing everything up a single commit (once we are happy with the state of it) and we just keep it open? Then people who want const generic support can easily cherry-pick it themselves and it should also be easier to rebase it if we make other changes (e.g. fixing the blake3 hashing). This way all your work is not lost, still usable and hopefully find it's way in some day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it is disappointing, I would have to agree. I'll do the final bits of cleanup and request another review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, is this really a regression? Or could we use this to trim down our digest list a bit (e.g., for all the Blake2s variants).