Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(pii): Use external utf16string crate #807

Merged
merged 1 commit into from
Oct 15, 2020

Conversation

flub
Copy link
Contributor

@flub flub commented Oct 13, 2020

The relay-wstring crate has been split off to the utf16string crate,
this switches our code over.

Unsurprisingly straightforward. Caveat is that this is not 100% the same code backing this: the external crate uses the ByteOrder trait to read and write u16s instead of using u16::from_le_bytes() and u16::to_le_bytes(). Because of that there's some more panicking code in there, but the perf impact of that seemed very small in microbenchmarks. And the whole thing has seen less review. Seems to work fine though.

#skip-changelog

The relay-wstring crate has been split off to the utf16string crate,
this switches our code over.
@flub flub requested a review from a team October 13, 2020 09:08
@@ -325,7 +328,7 @@ impl<'a> FusedIterator for WStrSegmentIter<'a> {}
/// longer match.
struct WStrSegment<'a> {
/// The raw bytes of this segment.
encoded: &'a mut WStr,
encoded: &'a mut WStr<LittleEndian>,
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to expose typedefs for this as typing out WStr<LittleEndian> every time is a little cumbersome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The shorthand is WStr<LE> which is also exposed as utf16string::LE. For signatures and struct definitions I choose the longer spelling as this isn't that common around here and makes reading easier when you're not used to it (i.e. myself in a year's time)

Copy link
Member

@jan-auer jan-auer Oct 15, 2020

Choose a reason for hiding this comment

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

Makes sense, being explicit here helps for sure. I was thinking of utf16string::{WStrL, WStrB} or something similar to avoid the angle brackets and additional imports (similar to how io::Result is a shorthand for Result<io::Error>), but in the end it doesn't really matter.

@jan-auer jan-auer merged commit dddfae3 into master Oct 15, 2020
@jan-auer jan-auer deleted the ref/utf16string-external-crate branch October 15, 2020 11:52
jan-auer added a commit that referenced this pull request Oct 15, 2020
* master:
  ref(pii): Use external utf16string crate (#807)
  feat(pii): Merge padding and masking characters (#810)
  fix(server): Rate limit outcomes emited only for events on "fast-path" (#809)
  fix(server): Rate limit outcomes emited only for events (#806)
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.

3 participants