All feature combinations compile #60
Conversation
| let ptr = v.as_ptr() as *const [u8; 16]; | ||
| Ok(Ulid::from_bytes(*unsafe { &*ptr })) | ||
| } | ||
| crate::ULID_LEN => Ulid::from_string(unsafe { core::str::from_utf8_unchecked(v) }) |
There was a problem hiding this comment.
Why is the bytes in v assumed to be valid utf8?
There was a problem hiding this comment.
It might be somewhat of an over-the-top optimization, but from_string(v: &str) only interacts with the string variable using v.len() and v.as_bytes() and doesn't actually use characters or char boundaries in any way. A better solution that still doesn't unnecessarily validate the string twice would have been to change base32::decode(_: &str) to base32::decode(_: &[u8]) and call that directly instead.
There was a problem hiding this comment.
Ah, i see, so because of line 79 deserializer.deserialize_str(UlidVisitor("an ulid string or value")) it forces the uft-8 to be valid. Understood thanks.
Out of curiosity what conditions need to be met for this code path to be hit?
Also would it be benificial to comment a safety block above here encompassing the reason this is safe.
I think it should be noted that you would need to be careful because if you add support to deserialize from bytes directly using serde this immediately becomes unsafe. Perhaps it might be worth while to validate the bytes anyways.
| @@ -32,11 +41,17 @@ mod test { | |||
| fn uuid_str_cycle() { | |||
| let uuid_txt = "771a3bce-02e9-4428-a68e-b1e7e82b7f9f"; | |||
| let ulid_txt = "3Q38XWW0Q98GMAD3NHWZM2PZWZ"; | |||
| let mut buf = uuid::Uuid::encode_buffer(); | |||
There was a problem hiding this comment.
Looks good.
Do you find this method useful? It might be nice to add something similar to Ulid.
| @@ -2,7 +2,8 @@ | |||
| name = "ulid" | |||
| version = "1.0.0" | |||
| authors = ["dylanhart <dylan96hart@gmail.com>"] | |||
| edition = "2018" | |||
| edition = "2021" | |||
There was a problem hiding this comment.
Is this strictly for resolver=2?
There was a problem hiding this comment.
I think this is for the question mark syntax in serde?/std and uuid?/std. I suspect it is needed for that, otherwise I might have changed it because there shouldn't be anything that supports the optional-dependency-feature syntax but doesn't support 2021.
It was previously possible to disable the default
stdfeature which caused the compilation to fail if theserdeoruuidfeatures were active. There were also tests which caused compilation errors when not building tests with thestdfeature.This will however require rust 1.60 or higher due to the use of weak dependency features (i.e.
std = ["rand", "serde?/std", "uuid?/std"]). This is a bump from the previous need for somewhere around 1.46-1.54 depending on compilation parameters. The documentation is also extended to include which features are needed for specificfns.The new implementation for
serdeno longer does any heap allocation, so it is completely safe to use in ano_stdcontext. It can also sneakily deserialize all reasonable kinds of data without the need to override withdeserialize_withif theDeserializersupports it.Fixes #57 and the specific use case in #52.