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

Use const fn instead of const macros #39

Merged
merged 6 commits into from Mar 25, 2020
Merged

Use const fn instead of const macros #39

merged 6 commits into from Mar 25, 2020

Conversation

sorpaas
Copy link
Member

@sorpaas sorpaas commented Mar 24, 2020

This removes field_const, field_storage_const, field_const_raw, affine_const, jacobian_const macros, and replaces them with const fns Field::new, FieldStorage::new, Field::new_raw, Affine::new, Jacobian::new.

@sorpaas
Copy link
Member Author

sorpaas commented Mar 24, 2020

If you want to review the large diffs of src/ecmult/const.rs and src/ecmult/const_gen.rs files, it's generated by make gen. (In the future we should change that to use build script though.)

@@ -11,5 +11,5 @@ matrix:
- beta
script:
- cargo build --verbose --all
- cargo test --verbose --all
- cargo test --verbose
Copy link
Member Author

Choose a reason for hiding this comment

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

The noconst feature is currently necessary for gen_ecmult and gen_genmult, but if it's enabled, then tests won't work, because nearly all user-facing functions depend on those constants. In the future, switching to build script will fix this.

Copy link

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

LGTM, it might be worth to add annotations to n (see inline)

src/field.rs Show resolved Hide resolved
@sorpaas sorpaas requested a review from drahnr March 24, 2020 17:14
@drahnr
Copy link

drahnr commented Mar 24, 2020

I am not sure if the failing tests are ok, I also did not review the generated files full of constants.

@sorpaas
Copy link
Member Author

sorpaas commented Mar 24, 2020

@drahnr Which tests are failing?

@drahnr
Copy link

drahnr commented Mar 24, 2020

Nevermind, looks good to me :)

@sorpaas sorpaas merged commit df8aaf1 into master Mar 25, 2020
@sorpaas sorpaas deleted the sp-const-fn branch March 25, 2020 02:44
trevor-crypto pushed a commit to monacohq/libsecp256k1 that referenced this pull request May 31, 2022
* Use const fn instead of const macros

* Build and test only main crate

* Still able to build all, but not test all

* Add description how Field n value is stored

* Clarify that the least signficant bit is in the front

* Bit -> byte
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

2 participants