Skip to content

extra_sizes implemented#229

Merged
tarcieri merged 2 commits intoRustCrypto:masterfrom
ycscaly:extra-sizes
May 8, 2023
Merged

extra_sizes implemented#229
tarcieri merged 2 commits intoRustCrypto:masterfrom
ycscaly:extra-sizes

Conversation

@ycscaly
Copy link
Contributor

@ycscaly ycscaly commented May 7, 2023

Resolves #228

src/uint.rs Outdated
(U8192, 8192, "8192-bit")
}

#[cfg(not(feature = "extra-sizes"))]
Copy link
Member

Choose a reason for hiding this comment

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

Instead of having a not, extra-sizes could be purely additive.

That would also take care of the doc_auto_cfg documentation, so the extra sizes are appropriately tagged as needing the feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem is, the original set of supported sizes is large and looks abit "random" IMO. If, e.g., we'd have U64, U128, U256, U512, U1024, U2048, U4096, U8192 as the types supported by default and everything else requiring extra_sizes, this would make sense. However, I don't think we can do that can we? (I'm not sure if feature-gating a non-feature gated type is considered a break of SemVer or not)

Otherwise, with the way things are currently, this becomes a little grotesque. However, if you'd still prefer that, I'll make the change. Remember however, that these distinctions will become harder with the extra_trait_impls macros.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Your suggestion was my original line of thought and I chose against it because of the aforementioned reasoning)

Copy link
Member

Choose a reason for hiding this comment

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

I think one way or another the types which require a feature need to be annotated with the feature name in the rustdoc.

If you simply define them in a module which is feature-gated, doc_auto_cfg should take care of that automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I keep only U64, U128, U256, U512, U1024, U2048, U4096, U8192 outside the feature-gate, or should I stick with the current set?

Copy link
Member

@tarcieri tarcieri May 8, 2023

Choose a reason for hiding this comment

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

No, that would be a breaking change, and other sizes are used by published crates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be good now

Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

Looks better now, thanks.

Still a lot in uint.rs, and I'm not sure the feature-gated sizes will be properly reflected in the rustdoc, but I can try fixing that in a followup.

@tarcieri tarcieri merged commit a459375 into RustCrypto:master May 8, 2023
@ycscaly
Copy link
Contributor Author

ycscaly commented May 8, 2023

Looks better now, thanks.

Still a lot in uint.rs, and I'm not sure the feature-gated sizes will be properly reflected in the rustdoc, but I can try fixing that in a followup.

Thanks!

@tarcieri tarcieri mentioned this pull request Sep 4, 2023
@Fethbita Fethbita mentioned this pull request Nov 19, 2025
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.

Add missing Uint<> types/sizes

2 participants