std::cmp::Ordering as {integer}

help
1

Is it alright to assume that the following assertions always hold? I assume the internal representation of std::cmp::Ordering is an implementation detail that could change any time?

fn main() {
    assert!((7.cmp(&9) as i32) < 0);
    assert_eq!(7.cmp(&9) as i32, -1);
}

(Playground)

2

I don't think so.

Looking at the definition...

... The enum has a #[repr(i8)] attribute and the overall type is stable.

That means you can rely on Ordering::Less being -1, Ordering::Equal being 0, and Ordering::Greater being 1. Furthermore, the i8 representation means the behaviour when casting to i32 or other types is also well-specified and not going to change.

3

The as i32 behaviour is visible to safe code on stable, so I think you're fine to assume it'll stay that way.

I wouldn't write any code that depends on the repr(i8), though. reprs aren't necessarily part of the public API, the same way that size_of isn't necessarily stable.

That said, why not just do

assert_eq!(7.cmp(&9), Ordering::Less);

instead?

EDIT (much, much later): While my general statement above is still correct, cmp::Ordering had an explicit libs-api FCP to make it part of the ABI, so you can absolutely rely on -1/0/+1 as i8 for that specific enum.

4

The release notes for 1.57 include the PR that added the repr, and comments on the PR seem to indicate this was intended to be a guaranteed, user-visible property.

1 Like
5

I was worried because rustdoc didn't show the values for the enum variants.

Does stable refer to the particular integer representation as well as the name of the enum variant?

That would mean,

#[stable(feature = "rust1", since = "1.0.0")]
#[repr(i8)]
pub enum Ordering {

Here, stable refers to enum Ordering but not to the repr annotation?

My use case is the following:

unsafe extern "C" fn compare_function<T>(a: *const lmdb::MDB_val, b: *const lmdb::MDB_val) -> c_int
where
    T: ?Sized + Storable,
{
    let a: &[u8] =
        slice::from_raw_parts((*a).mv_data as *const u8, (*a).mv_size.try_into().unwrap());
    let b: &[u8] =
        slice::from_raw_parts((*b).mv_data as *const u8, (*b).mv_size.try_into().unwrap());
    match T::cmp_bytes_unchecked(a, b) {
        Ordering::Less => -1,
        Ordering::Equal => 0,
        Ordering::Greater => 1,
    }
}
6

That's surprising. If they don't want people to make any assumptions about the type's layout, you would think they'd leave it as repr(Rust) and not provide any values for each enum variant :thinking:

1 Like
7

Well, the good news is that the repr is irrelevant to replacing that was as, so you're fine.

I was initially going to say that you should just leave the match and let LLVM deal with it, but it looks like LLVM doesn't notice that it's identity today. I filed Identity `match` on field-less enum compiles worse than using `as` · Issue #95313 · rust-lang/rust · GitHub about that.

But if you're really worried about assembly-peeking, you might want to add a cmp_bytes_unchecked_raw that returns a c_int, since normalizing to -1, 0, 1 for the enum adds some cost, compared to just returning the c_int from the memcmp I assume it's using internally.

This one definitely doesn't follow, because as i32 still works without the repr, so they might add the custom discriminants to enable that without intending to say anything about the layout.

It depends if they want to use it as an implementation detail for unsafe code elsewhere in the library.

See, for example, how it's still an FCP to say "it has the same layout as T" despite the repr(transparent) in the implementation: Clarify that ManuallyDrop<T> has same layout as T by joshlf · Pull Request #88375 · rust-lang/rust · GitHub

1 Like
8

I was hoping the match is zero-cost, but apparently it's not (yet).

Well, my default implementation uses std::cmp::Ord::cmp, so the return type of my implementation will be std::cmp::Ordering:

/// Types that can be stored
pub unsafe trait Storable: Ord {
    /* … */
    /// Pointer to aligned version of Self
    type AlignedRef<'a>: Deref<Target = Self> + PointerIntoOwned;
    /// Pointer to byte representation
    type BytesRef<'a>: Deref<Target = [u8]>
    where
        Self: 'a;
    /// Converts to byte slice
    fn to_bytes(&self) -> Self::BytesRef<'_>;
    /// Converts from byte slice
    unsafe fn from_bytes_unchecked(bytes: &[u8]) -> Self::AlignedRef<'_>;
    /// Compares byte representation
    unsafe fn cmp_bytes_unchecked(a: &[u8], b: &[u8]) -> Ordering {
        Self::from_bytes_unchecked(a).cmp(&Self::from_bytes_unchecked(b))
    }
}

Maybe it makes sense to add a corresponding note to std::cmp::Ordering's documentation, so the intention gets clear?

1 Like
9

Definitely. It's always good to send a PR adding the guarantee to the docs explicitly, and the libs-api can FCP a decision to say "yes, we'll guarantee that". Even for behaviour visible in safe code, like Document the order of {Vec,VecDeque,String}::retain by cuviper · Pull Request #60396 · rust-lang/rust · GitHub

10

I consider a repr to be part of the public API of structs if not documented otherwise. It often has to be, for repr(C) and repr(transparent), say.

11

Update on the missing optimization: C++ has the same problem (https://clang.godbolt.org/z/57jqWxbxe), so it's not rust's fault, and thus here's the bug on LLVM if you're curious to follow along

1 Like
12

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.