-
Notifications
You must be signed in to change notification settings - Fork 174
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
Use Diplomat &[&str]
, cmp::Ordering
, unvalidated UTF Writeable
#4786
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&[&str]
&[&str]
, cmp::Ordering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like @sffc to approve writeable changes
ffi.Pointer<_SliceUtf8> allocIn(ffi.Allocator alloc) { | ||
final slice = alloc<_SliceUtf8>(length); | ||
for (var i = 0; i < length; i++) { | ||
final codeUnits = Utf8Encoder().convert(_strings[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe extract the Utf8Encoder()
to a field? It is even a const constructor, so writing const Utf8Encoder encoder
could increase performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that actually necessary? I'd hope the compiler doesn't need to create an empty object to call the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if the compiler optimizes that, better to extract it and be sure IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer using const
.
Const constructors may be used to get a constant, but can also be used to instantiate a new object. Using the const keyword ensures it is a const. (And will in AOT ensure the object is dropped if the method doesn't actually use the object.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A const
field or a const
variable?
&[&str]
, cmp::Ordering
&[&str]
, cmp::Ordering
, unvalidated UTF Writeable
utils/writeable/src/utf.rs
Outdated
out.push_str(valid); | ||
out.push_str( | ||
char::REPLACEMENT_CHARACTER | ||
.encode_utf8(&mut [0; char::REPLACEMENT_CHARACTER.len_utf8()]), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: why not just use <String as core::fmt::Write>::write_char
and write_str
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because they're fallible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: use String::push(char)
for the second one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that has an extra branch which might not be optimized out. Probably cheapest to just use push_str
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed in godbolt that it does in fact get optimized out.
Co-authored-by: Shane F. Carr <shane@unicode.org>
); | ||
|
||
out.push_str(valid); | ||
out.push('�'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, optional: I would prefer explicitly writing out the code point so that I know this is a literal replacement character, U+FFFD, and not some other weird Unicode question mark symbol.
out.push('�'); | |
out.push('\u{FFFD}'); |
I say this aware of the following section in the style guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I recognise the question mark as the replacement character, I don't recognise the escape sequence.
No description provided.