rust-osdev / ucs2-rs

UCS-2 conversion utilities for Rust.
Mozilla Public License 2.0
6 stars 5 forks source link

Future of This Crate #13

Open phip1611 opened 1 year ago

phip1611 commented 1 year ago

From official documentation/comments it appears that ucs2 is a base for String handling in the uefi crate. However, there are exactly two usages in the output protocol implementation (output.rs).

The uefi-crate provides Char16 CString16 and CStr16 to work with UCS-2/UEFI strings, however.

I have the the following variants in mind:

I think A would be a good solution.

PS: Am I missing something? Is there a specific reason for the status quo?

Ping @nicholasbishop @GabrielMajeri

nicholasbishop commented 1 year ago

I'm in favor of option A as well. I assume that a lot of stuff has just ended up in uefi-rs just by inertia.

phip1611 commented 1 year ago

Nice. I can work on it in the next two weeks.

General question. The encoding looks way to complex, I think. From my perspective, the following encoding would be sufficient:


fn encode_char(char: char) -> UCS2Char {

  let unicode_codepoint = char as u32;
  if (unicode_codepoint <= 0xffff) {
    UCS2Char(unicode_codepoint)
  } else {
    panic!()
  }
}

Am I missing something?

PS: Oh, that is exactly how the Char16 type in the uefi crate does it :D

phip1611 commented 1 year ago

My ideas for version 0.4.0

GabrielMajeri commented 1 year ago

@phip1611 Thanks for the initiative! Option A sounds like a good idea. It fits the general Rust pattern of favoring small, independent crates instead of monolithic ones.

Besides the changes you've mentioned in the comment above, I'd also like to add a few minor clean-ups to the to-do list:

nicholasbishop commented 1 year ago

Sounds good. Could you up our access to this repo's settings? Right now I don't have permissions to do the branch rename or update branch protection rules.

GabrielMajeri commented 1 year ago

Sounds good. Could you up our access to this repo's settings? Right now I don't have permissions to do the branch rename or update branch protection rules.

Whoops, didn't realize there were permission issues. Should be fixed now.

nicholasbishop commented 1 year ago

Thanks. I've done the branch rename and added branch protection rules.