Skip to content

Future of This Crate #13

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

Open
phip1611 opened this issue Apr 2, 2023 · 7 comments
Open

Future of This Crate #13

phip1611 opened this issue Apr 2, 2023 · 7 comments

Comments

@phip1611
Copy link
Member

phip1611 commented Apr 2, 2023

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:

  • A: We should move Char16 CString16 and CStr16 from the uefi crate to this crate
  • B: drop this crate

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
Copy link
Member

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
Copy link
Member Author

phip1611 commented Apr 3, 2023

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
Copy link
Member Author

phip1611 commented Apr 3, 2023

My ideas for version 0.4.0

  • Let's wait until cstr[ing]16: convenience functions uefi-rs#751 and possible follow-ups are merged
  • Use rust edition 2021
  • remove all existing functionality as it is covered by the abstractions added in next step
  • move Char16, CStr16 and CString16 along will all tests to this crate
  • discuss whether we should stick to those names or something like UCS2String or just String so that people work with ucs2::String, ucs2::str and so on
  • release this
  • use it in uefi

@GabrielMajeri
Copy link
Collaborator

@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:

  • Rename the master branch to main - for consistency with the uefi-rs repo.
  • Activate branch protection rules for main (we already have CI checks, they're just not enforced before merging PRs). We might not actually want this since ucs-rs is a much smaller crate with less risk for breakage, but I'm not certain.
  • Create a CHANGELOG.md document and start using tags for labeling releases of the crate (again, not sure if we actually need this from the start, considering the smaller API of this library).

@nicholasbishop
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Member

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

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

No branches or pull requests

3 participants