-
Notifications
You must be signed in to change notification settings - Fork 309
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
Port unibyte-char-to-multibyte function #218
Conversation
rust_src/src/character.rs
Outdated
let mut c = ch.as_fixnum().unwrap() as u32; | ||
if c >= 0x100 { | ||
unsafe { | ||
error("Not a unibyte character: %d".as_ptr(), c); |
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 believe this should be error("Not a unibyte character, %d\0".as_ptr(), c)
, as Rust does not null-terminate it's string literals automatically.
Looks like the build is failing due to the release Other then my one inline comment, this looks like a solid PR 👍 |
@DavidDeSimone Thanks, updated |
rust_src/src/lisp.rs
Outdated
/// Check if Lisp object is a character or not. | ||
/// Similar to CHECK_CHARACTER | ||
#[inline] | ||
pub fn is_character_or_error(self) -> () { |
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.
Please make this as_character_or_error
. Then you can make the cast to fixnum
and ultimately to Codepoint
here already.
rust_src/src/multibyte.rs
Outdated
/// UNIBYTE_TO_CHAR macro | ||
#[inline] | ||
pub fn unibyte_to_char(cp: Codepoint) -> Codepoint { | ||
if (cp as u8).is_ascii() { |
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.
This cast is dangerous - it will truncate the codepoint so that thinks that aren't ascii will look like ascii.
rust_src/src/multibyte.rs
Outdated
#[inline] | ||
#[allow(unused_comparisons)] | ||
pub fn make_char_multibyte(cp: Codepoint) -> Codepoint { | ||
debug_assert!((cp) >= 0 && (cp) < 256); |
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.
You can just omit the >= 0
comparison, and remove the allow
.
rust_src/src/lisp.rs
Outdated
// Same as CHECK_TYPE macro, | ||
// order of arguments changed | ||
#[inline] | ||
#[allow(dead_code)] |
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.
Why is it dead code?
rust_src/src/character.rs
Outdated
#[lisp_fn] | ||
fn unibyte_char_to_multibyte(ch: LispObject) -> LispObject { | ||
ch.is_character_or_error(); | ||
let mut c = ch.as_fixnum().unwrap() as u32; |
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 need for mut
- just do either direct return or let
below.
rust_src/src/character.rs
Outdated
@@ -30,13 +30,11 @@ fn char_or_string_p(object: LispObject) -> LispObject { | |||
/// Convert the byte CH to multibyte character. | |||
#[lisp_fn] | |||
fn unibyte_char_to_multibyte(ch: LispObject) -> LispObject { | |||
ch.is_character_or_error(); | |||
let mut c = ch.as_fixnum().unwrap() as u32; | |||
let c = ch.as_character_or_error() as u32; |
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.
The "as u32" should be unnecessary (especially since Codepoint is just an alias for it 😄)
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.
yes it is, updated :)
f8cac5a
to
7f5c3d0
Compare
Conflicts resolved |
@shanavas786 OSX build fix is on master. If you merge in master travis should go green. |
@DavidDeSimone done :) |
Thanks! |
No description provided.