Skip to content
This repository was archived by the owner on Aug 15, 2021. It is now read-only.

Support 128-bit integers #145

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

nhynes
Copy link
Contributor

@nhynes nhynes commented Sep 1, 2019

This PR adds support for u128 and i128 in accordance with §2.4.2 of the CBOR spec.

@nhynes nhynes force-pushed the support-i128 branch 3 times, most recently from b07d2b7 to 8bc0bc6 Compare September 1, 2019 17:52
Copy link
Owner

@pyfisch pyfisch left a comment

Choose a reason for hiding this comment

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

I am not sure if I want to support big integers. They would be the first supported tagged value, additionally it breaks code that relies on the assumption that big integers are deserialized as bytestrings. (I don't know if there is such code)

Integer(i128),
/// Unsigned integer CBOR numbers.
Unsigned(u128),
Copy link
Owner

Choose a reason for hiding this comment

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

I don't like to have two overlapping integer variants (again) because a postive number can now be stored in two different ways.
Ist there something special about 128-bits that you need u128 numbers that do not fit into i128?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there something special about 128-bits that you need u128 numbers that do not fit into i128?

The only issue is with the canonical sort order. Before, i128 could record the sign and magnitude of the largest supported number. Since i128 is the largest data type, the sign isn't recorded independently of magnitude.

@nhynes
Copy link
Contributor Author

nhynes commented Sep 2, 2019

They would be the first supported tagged value

Well, it's in accordance with the spec and doesn't much affect the codebase.

Additionally it breaks code that relies on the assumption that big integers are deserialized as bytestrings.

Yes, that's a fair point. How about, instead, special-casing the implementation of Deserialize::deserialize_[iu]128? I'll update the PR.

@pyfisch
Copy link
Owner

pyfisch commented Sep 7, 2019

@sfackler What do you think about supporting 128-bit integers in this crate?

@@ -280,6 +280,33 @@ where
Ok(BigEndian::read_u64(&buf))
}

fn parse_bigint<V>(&mut self, signed: bool, visitor: V) -> Result<V::Value>

Choose a reason for hiding this comment

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

Is it better to separate this as a function rather than just keeping it down?

@grantperry
Copy link

@nhynes how hard would it be to fix your branch to include #206? It seems you may be able to wrap all ui128 functions in serde_if_integer! {...}?

@surban
Copy link

surban commented Jul 8, 2021

Is there something that we can do to move this forward? Being unable to serialize a basic Rust type unfortunately makes serde_cbor not usable for me.

@pyfisch
Copy link
Owner

pyfisch commented Jul 8, 2021

Is there something that we can do to move this forward?

You can pay me to work on this crate. Or you can find a company or team that wants to take over the maintenance of this crate.

Being unable to serialize a basic Rust type unfortunately makes serde_cbor not usable for me.

128-bit integers aren't part of CBOR (big ints are however) and 128-bit integers weren't part of Rust 1.0 as well. You can always write a custom (de)serializer for your data structures.

There is another serde CBOR crate: https://github.com/enarx/ciborium maybe it is right for your use-case. (They handle 128-bit integers.)

@pickfire
Copy link

pickfire commented Jul 8, 2021

Is there something that we can do to move this forward?

You can pay me to work on this crate. Or you can find a company or team that wants to take over the maintenance of this crate.

Or you can wait until someone who is interested to pick this up. At least I am not interested in this right now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants