Skip to content

Add trait impls for core::cmp::Ordering #102

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

Merged

Conversation

tarcieri
Copy link
Contributor

@tarcieri tarcieri commented Jan 10, 2023

Ordering is #[repr(i8)], making it possible to impl the following traits for it by casting to i8 (and back, where appropriate):

  • ConditionallySelectable
  • ConstantTimeEq
  • ConstantTimeGreater
  • ConstantTimeLess

@cosmicexplorer
Copy link

See #98 (comment); I think it's reasonable to impl ConditionallySelectable for Ordering in its own change here, but I think we should avoid impling ConstantTime{Eq,Less,Greater} for Ordering (see linked comment).

I'll note that #98 would also have very cleanly fixed https://github.com/RustCrypto/crypto-bigint/pull/159/files/d4d99aa7599123cdbb6e4c341bba7b7061e84f8b#r1066410790, which also hand-generates an Ordering instance. While I agree with your goal of separating the {Partial,}Ord traits from the ConditionallySelectable impl in #98, I think that impling ConditionallySelectable for Ordering is only useful for providing ConstantTime{Partial,}Ord.

If we want to slim down #98 more, I could try removing ConstantTimePartialOrd from it for now; that would allow us to address both libsignal and crypto-bigint's use cases cleanly without getting into the weeds of partial orders yet.

@tarcieri
Copy link
Contributor Author

Please confirm: I cannot see how implementing ConstantTime{Eq,Less,Greater} for Ordering can be used to implement ConstantTime{Partial,}Ord! The Ordering (or CtOption) instance cannot be created without the ConstantTime{Partial,}Ord traits in this PR. Is this correct?

No, I already had a code solution here:

https://github.com/RustCrypto/crypto-bigint/pull/159/files/d4d99aa7599123cdbb6e4c341bba7b7061e84f8b#r1064244105

However it would need the changes in this PR to compile.

It's pretty much the same one I suggested here:

#98 (comment)

I think having ConstantTime{Partial}Ord would still separately be useful, because it could be impl'd for core types much in the same way traits like ConstantTimeEq and ConditionallySelectable are impl'd for core types.

Separately, I don't see how you consume a returned Ordering without branching without this PR, especially in cases where you want to compare two or more orderings.

This type is `#[repr(i8)]`, making it possible to impl the following
traits for it by casting to `i8` (and back, where appropriate):

- `ConditionallySelectable`
- `ConstantTimeEq`
- `ConstantTimeGreater`
- `ConstantTimeLess`
@tarcieri tarcieri force-pushed the ordering-trait-impls branch from 258adb6 to c319c36 Compare February 7, 2023 15:00
@isislovecruft isislovecruft changed the base branch from main to develop February 27, 2023 22:17
@isislovecruft isislovecruft merged commit 1ac233f into dalek-cryptography:develop Feb 27, 2023
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.

3 participants