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

Add u64/u128 atom exports #127

Merged
merged 9 commits into from
Nov 17, 2023
Merged

Add u64/u128 atom exports #127

merged 9 commits into from
Nov 17, 2023

Conversation

sigilante
Copy link
Contributor

Necessary for floating-point; nice for conversions from direct atoms.

In particular, I'd like this expedited to complete last in the parsing jets.

Necessary for floating-point; nice for conversions from direct atoms.
@sigilante
Copy link
Contributor Author

Couldn't find the original as_u64 to cherry-pick in, lost in the mists of past branches...

rust/ares/src/noun.rs Outdated Show resolved Hide resolved
Comment on lines 475 to 483
pub unsafe fn as_u128_pair(self) -> Result<[u64; 2]> {
if self.size() <= 2 {
let u128_array = &mut [0u64; 2];
u128_array.copy_from_slice(&(self.as_slice()[0..2]));
Ok(*u128_array)
} else {
Err(Error::NotRepresentable)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rust has a native u128 type: https://doc.rust-lang.org/std/primitive.u128.html

You could make a 16-element byte array, copy into it from atom.as_bytes(), and then call u128::from_le_bytes() on the array to get the atom.

Suggested change
pub unsafe fn as_u128_pair(self) -> Result<[u64; 2]> {
if self.size() <= 2 {
let u128_array = &mut [0u64; 2];
u128_array.copy_from_slice(&(self.as_slice()[0..2]));
Ok(*u128_array)
} else {
Err(Error::NotRepresentable)
}
}
pub unsafe fn as_u128(self) -> Result<u128> {
if self.size() <= 2 {
let mut u128_array = [0u8; 16];
let self_bytes = self.as_bytes();
u128_array[0..self.as_bytes()].copy_from_slice(self_bytes);
Ok(u128::from_le_bytes(u128_array)
} else {
Err(Error::NotRepresentable)
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a pair of u64 to match the SoftFloat interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a generic u128, it's to solve a particular use case from urcrypt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

urcrypt urfloat, whatever we end up calling it

rust/ares/src/noun.rs Outdated Show resolved Hide resolved
rust/ares/src/noun.rs Outdated Show resolved Hide resolved
rust/ares/src/noun.rs Outdated Show resolved Hide resolved
rust/ares/src/noun.rs Show resolved Hide resolved
}
}

/* SoftFloat-compatible ordered pair of 64-bit words */
Copy link
Collaborator

Choose a reason for hiding this comment

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

doc comment please

@joemfb
Copy link

joemfb commented Nov 15, 2023

i think it's worth double checking here. I don't know anything about the softfloat crate in use, but vere's equivalent jets are clearly little-endian: https://github.com/urbit/vere/blob/629ac3698383be5c93324b5ed6355f4aac87dad9/pkg/noun/jets/e/rq.c#L57-L75

@sigilante
Copy link
Contributor Author

i think it's worth double checking here. I don't know anything about the softfloat crate in use, but vere's equivalent jets are clearly little-endian: https://github.com/urbit/vere/blob/629ac3698383be5c93324b5ed6355f4aac87dad9/pkg/noun/jets/e/rq.c#L57-L75

I'll check before we merge and comment here when that's done.

@sigilante
Copy link
Contributor Author

This test passes as written:

    #[test]
    fn test_u64() {
        let c = &mut init_context();

        assert_eq!(D(0x0).as_atom().unwrap().as_u64(), Ok(0x0_u64));
        assert_eq!(D(0x1).as_atom().unwrap().as_u64(), Ok(0x1_u64));
        assert_eq!(D(0x3fffffffffffffff).as_atom().unwrap().as_u64(), Ok(0x3fffffffffffffff_u64));

        assert_eq!(A(&mut c.stack, &ubig!(0xffffffffffffffffffffffffffffffff)).as_atom().unwrap().as_u64_pair().unwrap(), [0xffffffffffffffff_u64, 0xffffffffffffffff_u64]);
        assert_eq!(A(&mut c.stack, &ubig!(0xfffffffffffffffffffffffffffffffe)).as_atom().unwrap().as_u64_pair().unwrap(), [0xfffffffffffffffe_u64, 0xffffffffffffffff_u64]);
        assert_eq!(A(&mut c.stack, &ubig!(0x7fff8000000000000000000000000000)).as_atom().unwrap().as_u64_pair().unwrap(), [0x0_u64, 0x7fff800000000000_u64]);
    }

@sigilante
Copy link
Contributor Author

#[test]
fn test_u64() {
    let c = &mut init_context();

    assert_eq!(D(0x0).as_atom().unwrap().as_u64(), Ok(0x0_u64));
    assert_eq!(D(0x1).as_atom().unwrap().as_u64(), Ok(0x1_u64));
    assert_eq!(D(0x3fffffffffffffff).as_atom().unwrap().as_u64(), Ok(0x3fffffffffffffff_u64));

    assert_eq!(A(&mut c.stack, &ubig!(0xffffffffffffffffffffffffffffffff)).as_atom().unwrap().as_u64_pair().unwrap(), [0xffffffffffffffff_u64, 0xffffffffffffffff_u64]);
    assert_eq!(A(&mut c.stack, &ubig!(0xfffffffffffffffffffffffffffffffe)).as_atom().unwrap().as_u64_pair().unwrap(), [0xfffffffffffffffe_u64, 0xffffffffffffffff_u64]);
    assert_eq!(A(&mut c.stack, &ubig!(0x7fff8000000000000000000000000000)).as_atom().unwrap().as_u64_pair().unwrap(), [0x0_u64, 0x7fff800000000000_u64]);
    assert_eq!(A(&mut c.stack, &ubig!(0x00000000000000004fffffffffffffff)).as_atom().unwrap().as_u64_pair().unwrap(), [0x4fffffffffffffff_u64, 0x0_u64]);
}

indicates the direct atom pair case is swapped; correcting now. @joemfb

@sigilante
Copy link
Contributor Author

Marking as ready to merge. @eamsden

@eamsden
Copy link
Collaborator

eamsden commented Nov 17, 2023

@sigilante I would like @joemfb's review on this before merging. I'm not presently in a state where I can be confident in my own judgement of the code or the tests.

@eamsden eamsden requested a review from joemfb November 17, 2023 17:27
Copy link

@joemfb joemfb left a comment

Choose a reason for hiding this comment

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

this will do

@eamsden eamsden merged commit 3a4ebf3 into status Nov 17, 2023
1 check passed
@eamsden eamsden deleted the sigilante/as-u64s branch November 17, 2023 21:58
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