Skip to content

Conversation

@rooooooooob
Copy link
Collaborator

Previously the Int struct was statically defined. This causes issues
because some traits, wrappers, etc are only needed for certain runtime
configurations (e.g. preserve-encodings) or usages (e.g. if it's used as a key).

This also introduces more functionality like a wasm wrapper again and
conversion traits.

Also addresses issues with other primitive types e.g. i8, i64, u8, etc
and adds unit tests to cover all of them. (signed int deserialization was fundamentally broken and certain precisions for both signed and unsigned would lead to compile errors)

Fixes #55
Fixes #57

Previously the `Int` struct was statically defined. This causes issues
because some traits, wrappers, etc are only needed for certain runtime
configurations (e.g. preserve-encodings) or usages (e.g. if it's used as a key).

This also introduces more functionality like a wasm wrapper again and
conversion traits.

Fixes #55
Fixes #57

Also addresses issues with other primtive types e.g. i8, i64, u8, etc
and adds unit tests to cover all of them.

Fixes a few issues with wasm bindings for `Int`.

#[test]
fn signed_ints() {
// the + 1 is due to a bug in cbor_event that doesn't cover the entire range of i64
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably best to create an issue about this in the cbor_event repo and then link to the issue in this comment

* Option<T> for non-primitive T compiles properly now (doesn't use a
ref)

* removed TODO that was actually already implemented

* fix for maps in keys (OrderedHashMap now derives comparisons when
needed)
@rooooooooob
Copy link
Collaborator Author

rooooooooob commented Sep 20, 2022

I stuck a couple more fixes to this PR and removed that todo. Everything in babbage.cddl seems to compile now.

@SebastienGllmt
Copy link
Collaborator

Awesome! Can we create the issue in https://github.com/primetype/cbor_event so we can refer to it in our code and then merge this PR?

Now even with `preserve-encodings=false` we use the
`*negative_integer_sz()` functions in `cbor_event` as the non-`_sz` ones
have two issues: 1) they use `i64` as an interface and thus only cover
half of `nint`'s range and 2) there is a panic when serializing
`i64::MIN` so even for `i64` it is not complete.

See primetype/cbor_event#9

This commit also changes the `nint` type to be an unsigned `u64` to
cover the complete range and not allow in the rust code values from both
sides of the sign. This might be a little awkward it is represents
exactly the values possible with `nint`.

Negative constants are also added by this.
@rooooooooob
Copy link
Collaborator Author

I decided I might as well just use the explicit encoding endpoints even when preserve-encodings=false. That way when we generate CIP25 again without caring about encodings, we'll still be able to handle 100% of metadata, especially since this didn't throw, but instead panicked. I also added full nint support as we didn't handle deserializing negative constants nor did we support the full range (i.e. below i64::MIN) for nints.

I made the issue anyway though to document this in cbor_event: primetype/cbor_event#9

I also had forgotten to store the encoding fields on the new Int struct so that's included too in that last commit.

@SebastienGllmt SebastienGllmt merged commit 48b3093 into master Sep 21, 2022
@SebastienGllmt SebastienGllmt deleted the dynamic-int-generation branch September 21, 2022 02:31
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.

nints / bools compile errors int type conditional generation

3 participants