Support for no_std #4
Conversation
There was a problem hiding this comment.
Awesome stuff, thanks so much for taking the time to make this improvement.
The main feedback is around keeping the HashMap in std.
I know the original issue said to move to a BTreeMap but now that I think about it there's no reason to pay that cost in std.
Awesome stuff.
Cargo.toml
Outdated
| default = ["std"] | ||
| std = [] | ||
|
|
||
| [target.'cfg(not(feature = "std"))'.dependencies] |
There was a problem hiding this comment.
I'd love it if we could avoid introducing the extra dependency and just use the core crate. The core crate should work regardless of whether or not we are in no_std.
Since this crate is fairly small it hopefully isn't a hassle to just convert all of the std -> core.
There was a problem hiding this comment.
The reason I added it was because I couldn't figure out how to do use std::cmp::Ordering - do you have any idea what to do for this?
There was a problem hiding this comment.
actually the core seemed to work for that one, now I'm having trouble with Hash
src/grouped_rects_to_place.rs
Outdated
| @@ -19,9 +30,9 @@ where | |||
| { | |||
| // FIXME: inbound_id_to_group_id appears to be unused. If so, remove it. Also remove the | |||
| // Hash and Eq constraints on RectToPlaceId if we remove this map | |||
| pub(crate) inbound_id_to_group_ids: HashMap<RectToPlaceId, Vec<Group<GroupId, RectToPlaceId>>>, | |||
| pub(crate) inbound_id_to_group_ids: BTreeMap<RectToPlaceId, Vec<Group<GroupId, RectToPlaceId>>>, | |||
There was a problem hiding this comment.
// Top of file
#[cfg(std)]
use std::collections::{btree_map::Entry, BTreeMap as KeyValMap};
// Here
pub(crate) inbound_id_to_group_ids: KeyValMap<RectToPlaceId, Vec<Group<GroupId, RectToPlaceId>>>,
Then if not(std) KeyValMap can be an alloc::collections::BTreeMap
There was a problem hiding this comment.
Just noticed the part about alloc being the same as core in that std just re-exports it. Should be good to go now.
src/lib.rs
Outdated
| @@ -128,7 +141,7 @@ pub fn pack_rects< | |||
| box_size_heuristic: &BoxSizeHeuristicFn, | |||
| more_suitable_containers_fn: &ComparePotentialContainersFn, | |||
| ) -> Result<RectanglePackOk<RectToPlaceId, BinId>, RectanglePackError> { | |||
| let mut packed_locations = HashMap::new(); | |||
| let mut packed_locations = BTreeMap::new(); | |||
There was a problem hiding this comment.
Same thing in this file, KeyValMap and make it use HashMap in std and BTreeMap in no_std.
You can just define KeyValMap once in the crate root and everywhere else that's applicable use crate::KeyValMap
|
Okay, I think I made all the requested changes, let me know if anything is missing |
|
Looks great, thanks so much. |
|
Mind just squashing into one commit and rebasing then I'll merge? Thank you. |
src/bin_section.rs
Outdated
| cmp::Ordering, | ||
| fmt::{Debug, Display, Error as ErrorFmt, Formatter}, | ||
| }; | ||
| #[cfg(std)] |
There was a problem hiding this comment.
Oops I missed something.
std just relies on and re-exports core. So these cfg's are unnecessary.
This can just be
use core::{
cmp::Ordering,
fmt::{Debug, Display, Error as FmtError, Formatter},
};
Regardless of whether the std feature flag is enabled or not.
std only needs to control whether we use HashMap or BTreeMap as well as whether or not we impl std::err::Error for RectanglePackError
This applies to all of the files, and also applies to the alloc crate (std just re-exports alloc).
|
Thanks! |
|
Published to crates.io as |
Was trying to compile a bevy app to wasm and got this error, so instead of playing with feature flags on bevy I decided to practice my rust and try and close out this issue. #2