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

feat: support zeroize codegen #100

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Daniel-Aaron-Bloom
Copy link
Contributor

Also, I think derive_bits should enable derive. May be moot after the 1.60 bump.

Copy link
Member

@str4d str4d left a comment

Choose a reason for hiding this comment

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

Generally LGTM!

tests/derive_zero.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated
derive = ["byteorder", "ff_derive"]
std = ["alloc"]
# with MSRV 1.60 this could be merged into bits with ff_derive?/bits
# see PR#72 for more information.
derive_bits = ["bits", "ff_derive/bits"]
derive_bits = ["bits", "derive", "ff_derive/bits"]
derive_zero = ["zero", "derive", "ff_derive/zero"]
Copy link
Member

Choose a reason for hiding this comment

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

Unlike above, we do need the ability to enable the ff_derive feature flag for tests. I'm inclined to do so via a dedicated test feature flag:

Suggested change
derive_zero = ["zero", "derive", "ff_derive/zero"]
test-derive = ["derive", "ff_derive/zeroize"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with test_derive_zeroize, but I can change that if you want.

ff_derive/src/lib.rs Show resolved Hide resolved
ff_derive/src/lib.rs Outdated Show resolved Hide resolved
ff_derive/src/lib.rs Outdated Show resolved Hide resolved
ff_derive/src/lib.rs Outdated Show resolved Hide resolved
ff_derive/src/lib.rs Outdated Show resolved Hide resolved
@Daniel-Aaron-Bloom
Copy link
Contributor Author

I realized the MSRV 1.60 bump is easier than I thought, so if you merge #103 first, I can better cleanup the feature flags in this PR.

Also, let me know if you want me to add Daniel-Aaron-Bloom@485adc5 to this PR.

@str4d
Copy link
Member

str4d commented Dec 29, 2022

The MSRV 1.60 bump is indeed easy inside this crate, but imposes restrictions on the rest of the ecosystem. With this being a somewhat foundational crate, we want to be a bit more conservative with MSRV bumps (1.56 was necessary due to the bitvec dependency in the public API). So we're unlikely to bump to 1.60 until we've checked that our downstream users are happy with that (e.g. by themselves all having 1.60+ MSRVs).

@Daniel-Aaron-Bloom
Copy link
Contributor Author

Ping.

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.

2 participants