Add a helper TinyStr crate #8
Conversation
|
@raphlinus - would you be willing to review this? |
zbraniecki
force-pushed
the
tinystr2
branch
2 times, most recently
from
6bab977 to
1ed30c8
Compare
|
@zbraniecki I was suggesting this be a separate repo 😄, I think this crate has utility outside of unic-locale |
|
Also, crate name bikeshed: (tinystr may be more searchable) |
|
Yep! I'd like to first get a review of this code, before I land it - but I'll land it in a separate repo.
Yeaaa... I prefer searchability. I also am considering |
|
Wait, how does it allocate? |
Does it not? It owns the data it stores, right? |
|
It shouldn't allocate, it uses a scalar. So it can happily be #[no_std]. Filament is a superb physically based rendering engine, but I understand the appeal. |
It stores things inline in an integer |
|
Ah, ok. Seems like my understanding of terminology is fuzzy :( Sorry for that! I have one remaining open item from @raphlinus review in the previous issue: I'm not sure how to do that - I'm not familiar with BE/LE systems so I also don't know how to test it. |
I'm willing to desk-check, but here's the basic idea. The struct always stores in "native endianness" so that the transmute to A similar strategy is used to compute the various masks testing for letter classes, etc. This will have a small performance impact on BE systems, but makes it a lot easier to reason about. |
|
@raphlinus - thank you! Does it mean I should also do |
|
Yes, if (I haven't dug in) the intent is to provide a value that will be used by the constructor. The important thing is that |
|
Great! I added it to dumping and the unchecked constructor. I'll document it once we get to the docs phase. @raphlinus - would you have time to skim through the code and verify if it looks like you intended it to look like? I'll then separate it to a new repo and try to maintain although a lot of the math in it is above my skill level :) |
tinystr/src/helpers.rs
Outdated
| mask: u32, | ||
| ) -> Result<NonZeroU32, Error> { | ||
| // Mask is always supplied as little-endian. | ||
| let mask = mask.to_le(); |
There was a problem hiding this comment.
An incredibly minor nit, but this should be from_le. The semantics are the same, but it communicates the intent of converting from a little-endian to a native-endian representation.
tinystr/src/tinystr4.rs
Outdated
| let word = self.0.get().to_le(); | ||
| let mask = ((word + 0x3f3f_3f1f) & !(word + 0x2525_2505) & 0x8080_8080) >> 2; | ||
| let result = (word | mask) & !(0x20 & mask); | ||
| unsafe { TinyStr4(NonZeroU32::new_unchecked(result.to_le())) } |
There was a problem hiding this comment.
And this should be from_le as well, for the same reason.
| } | ||
| } | ||
|
|
||
| impl Into<u32> for TinyStr4 { |
There was a problem hiding this comment.
It's unclear whether this is intended to convert into the native or portable (little-endian) form. Code as written is native.
tinystr/src/tinystr8.rs
Outdated
| let mask = 0x80808080_80808080u64 >> (8 * (8 - len)); | ||
| // TODO: could do this with #cfg(target_endian), but this is clearer and | ||
| // more confidence-inspiring. | ||
| let mask = mask.to_le(); |
| } | ||
|
|
||
| impl Into<u64> for TinyStr8 { | ||
| fn into(self) -> u64 { |
|
Overall seems good to me aside from the endianness stuff |
|
Moved to a separate repo: https://github.com/zbraniecki/tinystr |
I separated out the new helper crate from #7, applied @raphlinus feedback and added benchmarks.