cxx-qt-lib: Add support for QUuid #1149
Conversation
|
This looks pretty in-depth, thanks! I think this is definitely worthy of a mention in CHANGELOG.md, you should add it there in this PR. I also started up the CI workflow, so we can see if that succeeds. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1149 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 71 71
Lines 11967 11967
=========================================
Hits 11967 11967 ☔ View full report in Codecov by Sentry. |
crates/cxx-qt-lib/src/core/quuid.cpp
Outdated
| } | ||
|
|
||
| QUuid | ||
| quuidNewV4() |
There was a problem hiding this comment.
Why is this renamed to V4?
IMHO, this seems like the normal creation function should be used by default when generating a new unique UUID.
There was a problem hiding this comment.
I've renamed these functions to match the Qt names, so create_uuid_v3, create_uuid, and create_uuid_v5. Is that the change you were looking for?
crates/cxx-qt-lib/src/core/quuid.rs
Outdated
| unsafe extern "C++" { | ||
| include!("cxx-qt-lib/qstring.h"); | ||
| type QString = crate::QString; | ||
| type QAnyStringView<'a> = crate::QAnyStringView<'a>; |
There was a problem hiding this comment.
Unfortunately QAnyStringView is only supported since Qt 6.
While it is possible to add #[cfg] flags to individual methods in CXX, it's apparently currently not possible to add them on type definitions (see: #1141 (comment) ).
So this will fail to compile with Qt5, which will remain supported for a little while longer.
As QAnyStringView is currently only used for the from_string method, maybe the easiest way forward here would be to make the quuidFromString method only accept a const QString& for now and add a from_any_string function later once this is sorted out? Maybe also rename from_string to from_qstring? That would keep the from_string name free.
There was a problem hiding this comment.
Makes sense. I've split it up into separate conversions from QStrings, Rust strs, and QByteArrays. Honestly, this way is cleaner anyway, because it's explicit about the types that can be converted rather than T: Into<QAnyStringView>.
crates/cxx-qt-lib/src/core/quuid.rs
Outdated
|
|
||
| /// Creates a UUID from its representation as a byte array in big endian. | ||
| pub const fn from_bytes(bytes: [u8; 16]) -> Self { | ||
| unsafe { mem::transmute::<[u8; 16], Self>(bytes) }.to_big_endian() |
There was a problem hiding this comment.
I have to admit it took me quite a while to understand this.
The function takes an array of big-endian bytes and then calls to_big_endian on it. That is quite confusing, as it's already big-endian.
I believe this still turns out to be correct, as the internal byte order is actually the host endianness, so calling to_big_endian will actually go from big to little endian, but only if the host order is little endian, which is exactly what's needed. But it's definitely not obvious that this is whats happening.
So at the very least this needs an explanation comment.
There was a problem hiding this comment.
I've added explaining documentation and directly copied over the naming convention and documentation for u8::to_be, so that it will be more familiar to people who have worked with endian conversions. Do these changes make it sufficiently easy to understand? Is there anything else I should add?
There was a problem hiding this comment.
Hi @jnbooth ,
thank you for taking the time to work on this.
UUIDs are an important topic and having QUuid in cxx-qt-lib would be great!
I have left some minor nitpicks, but the most important problem is going to be Qt5 support, as QAnyStringView isn't supported on Qt5.
For now I'm okay to only support QString so we can merge this PR earlier.
Co-authored-by: Leon Matthes <[email protected]>
Co-authored-by: Leon Matthes <[email protected]>
Adds a shared type for QUuid, which can optionally be converted to and from the type provided by the uuid crate. Since it's expensive to convert between Strings and QStrings, QUuids provide a convenient no-allocation alternative for identifiers.