search

tats-u / rust-oem-cp

Rust library that handles OEM code pages (e.g. CP{437,737,850}) for single byte character sets
MIT License
5 stars 3 forks source link

Migrate to `phf`, add `no-std` support #9

Closed mkroening closed 1 year ago

mkroening commented 1 year ago

Closes https://github.com/tats-u/rust-oem-cp/issues/7.

This PR is structured as follows:

  1. Migrate code generation from generate_table.py to build.rs.

    Before, generate_table.py was called once and the generated code was then committed to the repository. Now, build.rs generates the code from assets/code_tables.json on the fly for every user before the compilation. The resulting code is not saved persistently anywhere, and is regenerated after cargo clean.

    This step does not change the generated code or any dependencies.

  2. Migrate to phf.

    This change migrates the code generation for hash maps from the manual lazy_static and ahash::AHashMap based code to a phf_codegen and phf::Map based one.

This commit and the following ones are for supporting no-std environments:

  1. Enable Rust 2021 to enable Cargo resolver version 2.

    Without this, phf_codegen in build-dependencies enables the std feature for phf_shared in the normal dependencies scope as well.

    See https://doc.rust-lang.org/cargo/reference/features.html#feature-resolver-version-2

The next two commits are for refactoring all functionality that requires an allocator into one module for easier gating.

  1. Move string functions into string module.

  2. Move TableType string methods to string module.

  3. Add no_std support.

    This makes all functionality depend on the alloc crate instead of std, making it suitable for no-std environments. Additionally, the alloc functionality can be disabled via Cargo features for no-std environments without alloc.

tats-u commented 1 year ago

Are you trying to rewrite generate_table.py in Rust? If the build.rs does just what generate_table.py, I think you should rename build.rs as generate_tables.rs for example. build is too generic.

mkroening commented 1 year ago

Are you trying to rewrite generate_table.py in Rust? If the build.rs does just what generate_table.py, I think you should rename build.rs as generate_tables.rs for example. build is too generic.

build.rs is a Cargo build script. It has to be called build.rs. It's executed before compilation of the source files, and I used it to replace generate_table.py in the first commit, since I deemed it most suitable for migrating to phf in the second commit.

I rebased on latest master and applied your early suggestions. I pushed it commit by commit to have CI runs for each one of them. I'll also update the description of this PR and mark this as ready for review.

Would you like me to split this PR up or are you okay with keeping it this big?

mkroening commented 1 year ago

If the build.rs does just what generate_table.py, I think you should rename build.rs as generate_tables.rs for example. build is too generic.

I just found, that you can actually use a different name for the build script: package.build. I have never seen another name than build.rs before though, and I would find things easier myself, if we keep the name build.rs for the build script.

It's up to you to decide, though. :)

tats-u commented 1 year ago

@mkroening I see, and you can go ahead (you can stick to build.rs) if you add sufficient comments.

mkroening commented 1 year ago

Sorry for taking so long. I addressed your concerns. :)

tats-u commented 1 year ago

This change is now available in 1.2.0. Sorry to keep you waiting for a long time.

mkroening commented 1 year ago

No worries! Thanks a lot! :)

tats-u commented 1 year ago

Sorry I'd like you to use 2.0.0(-beta.1) instead. I found this patch is a breaking change, contrary to my expectations.