Little-Endian byte order support #277

Closed
opened 2018-07-06 12:23:12 -05:00 by awakecoding · 15 comments
awakecoding commented 2018-07-06 12:23:12 -05:00 (Migrated from github.com)

Microsoft uses little-endian byte order representation for its GUID structure in memory, like this:

typedef struct _GUID { UINT32 Data1; UINT16 Data2; UINT16 Data3; BYTE Data4[8]; } GUID, UUID, *PGUID, *LPGUID, *LPCGUID;

Would it be possible to add the option of reading from byte arrays in little endian order, and writing to byte arrays in little endian order? The current code apparently assumes big-endian ordering 100% of the time, making it harder to interoperate in specific cases.

For instance, from_bytes_le and as_bytes_le could be special functions to read/write to/from bytes in little-endian order.

Microsoft uses little-endian byte order representation for its GUID structure in memory, like this: `` typedef struct _GUID { UINT32 Data1; UINT16 Data2; UINT16 Data3; BYTE Data4[8]; } GUID, UUID, *PGUID, *LPGUID, *LPCGUID; `` Would it be possible to add the option of reading from byte arrays in little endian order, and writing to byte arrays in little endian order? The current code apparently assumes big-endian ordering 100% of the time, making it harder to interoperate in specific cases. For instance, from_bytes_le and as_bytes_le could be special functions to read/write to/from bytes in little-endian order.
Dylan-DPC-zz commented 2018-07-09 14:09:53 -05:00 (Migrated from github.com)

@awakecoding Thanks for proposing this. You can go ahead and write a PR if you think you can work on it else one of us will do it.

@awakecoding Thanks for proposing this. You can go ahead and write a PR if you think you can work on it else one of us will do it.
kinggoesgaming commented 2018-07-09 14:55:24 -05:00 (Migrated from github.com)

If you do open a PR, please target breaking branch. (we will soon be merging it into master but for now we still have active development of breaking)

If you do open a PR, please target `breaking` branch. (we will soon be merging it into `master` but for now we still have active development of `breaking`)
awakecoding commented 2018-07-09 15:02:35 -05:00 (Migrated from github.com)

@Dylan-DPC @kinggoesgaming I don't know if I should try to do it myself or let you decide how it should be done, as I am unsure how you would like this to be implemented in a proper manner. Do you have a preference for the new API? I don't mind if the internal representation remains in big endian, as long as we can easily import/export to/from little-endian order bytes.

@Dylan-DPC @kinggoesgaming I don't know if I should try to do it myself or let you decide how it should be done, as I am unsure how you would like this to be implemented in a proper manner. Do you have a preference for the new API? I don't mind if the internal representation remains in big endian, as long as we can easily import/export to/from little-endian order bytes.
kinggoesgaming commented 2018-07-09 15:25:26 -05:00 (Migrated from github.com)

You are always welcome to open to a PR. If we find something that can be improved, we can leave relevant comments.

Edit:

I think I agree with your assumption that internal representation should be left as-is, and only constructors should allow Little Endian.

You are always welcome to open to a PR. If we find something that can be improved, we can leave relevant comments. Edit: I think I agree with your assumption that internal representation should be left as-is, and only constructors should allow Little Endian.
kinggoesgaming commented 2018-07-16 21:28:08 -05:00 (Migrated from github.com)

The other way we can do this is making byteorder a public dependency and using its Endian structs. We can default to using byteorder::BigEndian and as needed, the library user can use byteorder::LittleEndian instead.

But I am doubtful if this will be sound, as this may accidently bump our minimum rust version needed to 1.26. (This is on top of my head, I haven't test this yet)

The other way we can do this is making `byteorder` a public dependency and using its Endian structs. We can default to using `byteorder::BigEndian` and as needed, the library user can use `byteorder::LittleEndian` instead. But I am doubtful if this will be sound, as this may accidently bump our minimum rust version needed to `1.26`. (This is on top of my head, I haven't test this yet)
BurntSushi commented 2018-07-17 05:38:36 -05:00 (Migrated from github.com)

@kinggoesgaming Note that byteorder's MSRV is 1.12 currently.

Speaking from experience, I'd probably prefer solutions that don't incur additional public dependencies.

@kinggoesgaming Note that `byteorder`'s MSRV is 1.12 currently. Speaking from experience, I'd probably prefer solutions that don't incur additional public dependencies.
awakecoding commented 2018-07-17 07:36:39 -05:00 (Migrated from github.com)

I agree that introducing a new public dependency is probably not a good idea

I agree that introducing a new public dependency is probably not a good idea
kinggoesgaming commented 2018-07-17 07:41:35 -05:00 (Migrated from github.com)

@BurntSushi we already depend on byteorder for u128 to Uuid conversion and does mean minimum version of 1.26

@BurntSushi we already depend on `byteorder` for `u128` to `Uuid` conversion and does mean minimum version of 1.26
BurntSushi commented 2018-07-17 07:44:29 -05:00 (Migrated from github.com)

Oh, okay. I guess I got confused. Sounded like MSRV and byteorder as a public dep were related. But if you are unconditionally using u128 internally, then yeah, your MSRV has nothing to do with byteorder really.

Oh, okay. I guess I got confused. Sounded like MSRV and byteorder as a public dep were related. But if you are unconditionally using u128 internally, then yeah, your MSRV has nothing to do with byteorder really.
lzybkr commented 2018-10-03 17:27:06 -05:00 (Migrated from github.com)

Related - it would be handy to support the winapi-rs crate by implementing std::convert::From<GUID>.

Related - it would be handy to support the winapi-rs crate by implementing `std::convert::From<GUID>`.
kinggoesgaming commented 2018-10-03 22:15:26 -05:00 (Migrated from github.com)

Related - it would be handy to support the winapi-rs crate by implementing std::convert::From<GUID>.

opened a new issue #333

> Related - it would be handy to support the winapi-rs crate by implementing `std::convert::From<GUID>`. opened a new issue #333
KodrAus commented 2018-10-30 02:55:29 -05:00 (Migrated from github.com)

The current code apparently assumes big-endian ordering 100% of the time, making it harder to interoperate in specific cases.

I thought this was the case too, but it turns out the current code doesn't make any assumptions about endianness so will read and write the fields of the uuid in the platform's native endianness. For most users that means it's probably already little-endian. Because of this our current versions of the recently added *_be and *_le methods aren't quite correct yet.

For interoperability I would kind of expect us to pick an endianness when storing our fields and stick with it when reading to and from them (that's what I was expecting already). Then we'd really only need the from_bytes_* and as_bytes_* for integrating with other systems that have their own representations of uuid that assume some specific endianness. When working with from_fields and to_fields I think we should just always convert to and from the platform's native endianness. So in the Windows case we're always going to create a Guid using little endian fields because Windows happens to have no interest in supporting any big endian architectures.

I think we'd have to be careful about making the decision of picking an explicit endianness going forward though. That could break folks storing uuids as bytes and treating them as fields in some surprising ways...

> The current code apparently assumes big-endian ordering 100% of the time, making it harder to interoperate in specific cases. I thought this was the case too, but it turns out the current code doesn't make any assumptions about endianness so will read and write the fields of the uuid in the platform's native endianness. For most users that means it's probably already little-endian. Because of this our current versions of the recently added `*_be` and `*_le` methods aren't quite correct yet. For interoperability I would kind of expect us to pick an endianness when storing our fields and stick with it when reading to and from them (that's what I was expecting already). Then we'd really only need the `from_bytes_*` and `as_bytes_*` for integrating with other systems that have their own representations of uuid that assume some specific endianness. When working with `from_fields` and `to_fields` I think we should just always convert to and from the platform's native endianness. So in the Windows case we're always going to create a `Guid` using little endian fields because Windows happens to have no interest in supporting any big endian architectures. I think we'd have to be careful about making the decision of picking an explicit endianness going forward though. That could break folks storing uuids as bytes and treating them as fields in some surprising ways...
KodrAus commented 2018-11-05 21:12:31 -06:00 (Migrated from github.com)

Sorry, I was totally mistaken here. We are just assuming the input integers to from_fields are big endian, so all that needs to be done is to make the little endian methods properly shift their bytes.

Sorry, I was totally mistaken here. We are just assuming the input integers to `from_fields` are big endian, so all that needs to be done is to make the little endian methods properly shift their bytes.
kinggoesgaming commented 2019-01-31 14:13:41 -06:00 (Migrated from github.com)

closing

closing
thedrow commented 2019-05-13 05:18:25 -05:00 (Migrated from github.com)

I just had to use the code here:
https://github.com/uuid-rs/uuid/pull/330/files#diff-b4aea3e418ccdb71239b96952d9cddb6R1109

I rather have this as a public API if possible.

GitHub
I'm submitting a feature Description This PR adds little-endian constructors as well as a to_bytes_le and a to_fields_le method. I've also renamed the previous bytes methods and appended th...
I just had to use the code here: https://github.com/uuid-rs/uuid/pull/330/files#diff-b4aea3e418ccdb71239b96952d9cddb6R1109 I rather have this as a public API if possible. <blockquote><img src="https://avatars0.githubusercontent.com/u/3757771?s=400&v=4" width="48" align="right"><div><img src="https://github.githubassets.com/favicon.ico" height="14"> GitHub</div><div><strong><a href="https://github.com/uuid-rs/uuid/pull/330">Add little-endian functions by Veykril · Pull Request #330 · uuid-rs/uuid</a></strong></div><div>I&#39;m submitting a feature Description This PR adds little-endian constructors as well as a to_bytes_le and a to_fields_le method. I&#39;ve also renamed the previous bytes methods and appended th...</div></blockquote>
Sign in to join this conversation.
No milestone
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-uuid-rs/uuid#277
No description provided.