Skip to content

Conversation

@exg
Copy link
Collaborator

@exg exg commented May 17, 2025

No description provided.

Copy link

@rodionterekhin rodionterekhin left a comment

Choose a reason for hiding this comment

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

I'm no pro in Rust, but everything looks good to me. One question though: why do we use exactly 128 as an intermediate value for an extension tag (mapped to -1 in serialize_newtype_variant)? Probably tag: i8 is not an ideal type choice

@exg
Copy link
Collaborator Author

exg commented May 21, 2025

I'm no pro in Rust, but everything looks good to me. One question though: why do we use exactly 128 as an intermediate value for an extension tag (mapped to -1 in serialize_newtype_variant)? Probably tag: i8 is not an ideal type choice

That is because extensions are serialized using serialize_newtype_variant with the tag given as the variant_index argument, whose type is unsigned.

@rodionterekhin
Copy link

I'm no pro in Rust, but everything looks good to me. One question though: why do we use exactly 128 as an intermediate value for an extension tag (mapped to -1 in serialize_newtype_variant)? Probably tag: i8 is not an ideal type choice

That is because extensions are serialized using serialize_newtype_variant with the tag given as the variant_index argument, whose type is unsigned.

I mean that it could be possible to change type of tag member of ExtSerializer struct to u8 (and subsequently tag parameter and of msgpack::ext::write_ext and its body). This change will not break previous code for user-defined extensions, because their tags are guaranteed to be lower than 128, thus no overflow during type conversion happens (because of 0..127 bounds check and try_into) and no change to value will be when tag type is changed to unsigned.

But according to this logic it would be also possible to avoid using value 128 as an alias for -1 extension type by directly passing 255 to serialize_newtype_variant, which corresponds more to 0xFF being written to buffer, but will require some refactoring, and I'm unsure whether it should be done in this PR or not (and is it really worth it or not).

@exg
Copy link
Collaborator Author

exg commented May 22, 2025

I mean that it could be possible to change type of tag member of ExtSerializer struct to u8 (and subsequently tag parameter and of msgpack::ext::write_ext and its body).

i8 is the correct type, as the extension type tag is an integer between -128 and 127 and is serialized as an 8-bit signed integer. The reason for the intermediate mapping is that the extension type needs to be represented in the Serde data model. The current code serializes extension types as if they were variants of the type

enum Ext {
    Type0(&[u8]),
    Type1(&[u8]),
    ...
    Type127(&[u8]),
    Timestamp(&[u8]),
}

and derives the tag from the variant index. It is not pretty, but other alternatives are worse in that they require more boilerplate code.

@rodionterekhin
Copy link

i8 is the correct type, as the extension type tag is an integer between -128 and 127 and is serialized as an 8-bit signed integer. The reason for the intermediate mapping is that the extension type needs to be represented in the Serde data model. The current code serializes extension types as if they were variants of the type

enum Ext {
    Type0(&[u8]),
    Type1(&[u8]),
    ...
    Type127(&[u8]),
    Timestamp(&[u8]),
}

and derives the tag from the variant index. It is not pretty, but other alternatives are worse in that they require more boilerplate code.

Ok, I see it now. Thank you for clarifying

exg added 3 commits May 24, 2025 10:18
Signed-off-by: Emanuele Giaquinta <emanuele.giaquinta@gmail.com>
Signed-off-by: Emanuele Giaquinta <emanuele.giaquinta@gmail.com>
Signed-off-by: Emanuele Giaquinta <emanuele.giaquinta@gmail.com>
@exg exg merged commit 6e2ea60 into ormsgpack:master May 24, 2025
44 checks passed
@exg exg deleted the timestamp-ext branch May 24, 2025 10:05
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.

2 participants