RazrFalcon / roxmltree

Represent an XML document as a read-only tree.
Apache License 2.0
434 stars 37 forks source link

Fallback for platforms without pointer atomics #110

Closed dragazo closed 1 year ago

dragazo commented 1 year ago

alloc::sync::Arc only exists on platforms with #[cfg(target_has_atomic = "ptr")], so this crate fails to compile on a lot of embedded platforms. If we want to support these platforms, we could fall back to alloc::rc::Rc on platforms that lack pointer atomics.

Alternatively, a feature flag could enable this - like maybe we have a default feature flag called sync that uses alloc::sync::Arc and users can explicitly disable default features to access the alloc::rc::Rc version instead. The idea being that, even on platforms with pointer atomics, some users might want Rc anyway since it's a bit faster than Arc at the expense of losing Send.

RazrFalcon commented 1 year ago

Cargo features must be additive. Meaning that if we add the sync feature that flips that storage container - we could get build conflicts when one dependency uses sync and other isn't. So I guess cfg is our only choice here.

No idea how to test this case on CI, but it should be fine for now.

RazrFalcon commented 1 year ago

Great, and target_has_atomic requires MSRV of 1.60 ...

dragazo commented 1 year ago

The following command could be used to test this in CI. It uses cross to build for a target that we know doesn't support pointer-sized atomics. In a github action, you can just add use-cross: true to a build action that uses actions-rs/cargo@v1 instead of manually installing cross.

cross +nightly build --target armv4t-none-eabi --no-default-features -Z build-std
dragazo commented 1 year ago

One fix for the MSRV issue I've done in the past was to conditionally apply that attr based on the rust version:

#[rustversion::attr(since(1.60), cfg(target_has_atomic = "ptr"))]

That way this crate can compile on recent rust versions, and only fails on old rust toolchains that specifically target the somewhat rare platforms that lack pointer atomics.

RazrFalcon commented 1 year ago

Does this change works for you?

We can ignore CI for now. It would be hard to accidentally broke it.

rustversion is not part of Rust, but an external crate. Too complicated.

dragazo commented 1 year ago

Yep, that should work for me, thanks! The MSRV bump is unfortunate, sorry about that.