Add no_std support#72
Conversation
- Switched to `hashbrown::HashMap` from `std::collections::HashMap`. This dependency was already in-tree and is only used internally, so no public facing change. - Switched to deriving `thiserror::Error` instead of manual `Error` implementation. This is done to preserve the 1.65 MSRV (since `thiserror` correctly handles choosing between `core::error::Error` and `std::error::Error` based on compiler version). This dependency is used in tests already, but is now a proper dependency. - Ensured `saphyr-parser` and `saphyr` compile for `no_std` targets such as `wasm32v1-none` and `x86_64-unknown-none`. Added this to CI to ensure compatibility doesn't regress.
|
CI failure was caused by attempting to compile tools, benchmarks, etc. for |
|
I usually run |
Ethiraric
left a comment
There was a problem hiding this comment.
I'd just like 2 changes to that and I'll merge:
- Add an entry to the
CHANGELOG.md, especially describing the breaking change you mentioned. - Add the
cargo checkforno_stdto thejustfile.
Thanks for your contribution! <3 It's always appreciated to have folks dedicated to broadening usages!
Done!
I've added a new
Happy to help! |
|
All good, thanks a lot! |
Objective
Add
no_stdsupport while preserving the API as-is as much as possible. Since this project is still in an early pre-1.0 form, addingno_stdsupport now will avoid possibly breaking changes later in its life.Solution
alloc::collections::BTreeMapfromstd::collections::HashMap.hashbrowncould be used instead, but since it is not currently included insaphyr-parser's dependency graph, I think it is preferable to useBTreeMap.thiserror::Errorinstead of manualErrorimplementation. This is done to preserve the 1.65 MSRV (sincethiserrorcorrectly handles choosing betweencore::error::Errorandstd::error::Errorbased on compiler version). This dependency is used in tests already, but is now a proper dependency. As an alternative, MSRV could be increased to the point whereErrorincoreis stable, or abuild.rscould be added which uses the same technique asthiserror. Just usingthiserroris the least intrusive option in my opinion.saphyr-parserandsaphyrcompile forno_stdtargets such aswasm32v1-noneandx86_64-unknown-none. Added this to CI to ensure compatibility doesn't regress.sahpyr::loader::LoadErrorasnon_exhaustiveto handle theIOvariant inno_stdcontexts. This is the only breaking change in this PR.coreandallocinstead ofstdwhere possible. Note that this implies that the crates are notno_alloccompatible (a much harder hurdle to clear).hashlinkfromsaphyr-parser(it is currently unused) andarraydequefromsahpyr(likewise). These areno_stdcompatible, but they are unused and so just increase compilation time.Notes
serdeis alsono_stdcompatible, so a future version of this crate withserdecompatibility can still beno_std.stdfeatures to eithersaphyrorsaphyr_parser. The existingdebug_printsandencodingfeatures perfectly isolate thestdfunctionality already, so an extra feature wouldn't really add anything.no_stdusing#![no_std], rather than usingcfg_attr(...)to conditionally enableno_stdsupport. This is a deliberate choice to avoid changes in the implicit prelude, which can cause extremely annoying CI failures based on feature combinations.alloc::sync::Arcis currently feature-gated onsaphyr/encoding, this has the added benefit of supporting platforms with partial atomic support (e.g., Raspberry Pi Pico, etc.). In the future, ifArcis required even inno_stdcontexts, I would recommend usingportable_atomicandportable_atomic_utilto maintain support for those platforms.