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

Tiny-const-generics #255

Merged
merged 3 commits into from
Apr 17, 2021
Merged

Conversation

huitseeker
Copy link
Contributor

@huitseeker huitseeker commented Mar 28, 2021

This is an easy initial fragment of #186 aimed at breaking off one easy part out of that big PR, and get us started on the path to min_const_generics. Props to @jon-chuang for the actual work.

(#186 needs a bit of bespoke specialization to achieve performance parity, so this just aims for code simplification)

bench on master:
https://gist.github.com/413013073c2c81e8999e77c1cb37c5b8
bench on this PR:
https://gist.github.com/1f1cc7a70d02709889b9a0dfa180f2cd

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

Untested ACK

@ChenxingLi
Copy link

Have you planned to replace the macro bigint_impl! with the const-generics trait?

@huitseeker
Copy link
Contributor Author

@ChenxingLi The work in that direction is ongoing in #186, I believe.

@Pratyush
Copy link
Member

@ValarDragon are we okay with bumping the next release to require a minimum supported rust version of 1.51?

@ValarDragon
Copy link
Member

Isn't rust 1.51 only a week old? imo, its overly prohibitive for a library to be backwards incompatible with versions that aren't even a month old. cref #215 #219.

Perhaps we should only depend on 1.51 after 6 weeks or so, when 1.52 comes out? And until then maintain a 1.51 branch?

@ChenxingLi
Copy link

@ChenxingLi The work in that direction is ongoing in #186, I believe.

Thanks! That's cool.

@ValarDragon
Copy link
Member

I made a compiler_1.51 branch, lets switch this to point at that branch, and merge into there? (And then plan to merge that branch into master once the rust compiler hits 1.53)?

@huitseeker huitseeker changed the base branch from master to compiler_1.51 April 6, 2021 16:53
@weikengchen
Copy link
Member

Dev, you mentioned 1.53 in the last reply, is it 1.51? https://blog.rust-lang.org/2021/03/25/Rust-1.51.0.html Or we just want to wait until 1.53 so it is more commonly used?

@ValarDragon
Copy link
Member

ValarDragon commented Apr 17, 2021

The 1.51 branch was meant to indicate targeting that MSRV, and then we'd want to hold off on merging that into master until 1.53.

I think this is good to merge into the 1.51 branch now.

@weikengchen
Copy link
Member

weikengchen commented Apr 17, 2021

Sure, we can do it soon. I am looking at the code. Do you think the mentioning of 1.51+ in README.md and the version check necessary then?

@ValarDragon
Copy link
Member

I think so, it will make the MSRV clearly documented, and if a compilation error occurs from someone using an insufficient version, it will provide a clear error message.

@weikengchen weikengchen merged commit 8df29c4 into arkworks-rs:compiler_1.51 Apr 17, 2021
ValarDragon added a commit that referenced this pull request May 12, 2021
* updates bytes to use const generics

* Add a check for a minimum rustc version in ff

* Specify min version for the compiler in README

Co-authored-by: François Garillot <4142+huitseeker@users.noreply.github.com>
Co-authored-by: Weikeng Chen <w.k@berkeley.edu>
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.

6 participants