Add no_std support to bevy_reflect#16256
Conversation
|
Tested with my demo UEFI application and confirmed it works as expected, letting me derive |
|
IIUC it's possible to make |
|
I'll give this another look to see if |
|
Actually, reflection uses a lot of |
MrGVSV
left a comment
There was a problem hiding this comment.
Overall looks great! A lot less involved than I thought it'd be.
Just a few questions (mainly for my own clarity and understanding).
| slice::Iter, | ||
| }; | ||
|
|
||
| #[cfg(not(feature = "std"))] |
There was a problem hiding this comment.
Why do we use this attribute here but not on any of the other alloc imports?
There was a problem hiding this comment.
So this is an interesting one. Basically I added these imports without the feature gate to ensure no_std compiled, and then checked compilation with std. In the cases were Clippy said these imports weren't used I added the feature gate. So in short, I don't have a good explanation as to why sometimes it's needed and sometimes it's not, but the process used to arrive here was very mechanical ("clippy said so")
…entation Co-Authored-By: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
Instead of embedding an `extern crate alloc;` Co-Authored-By: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
Moved `std` feature gate inside `impl_reflect_for_atomic` explicitly for `ReflectDe/Serialize` Co-Authored-By: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
Co-Authored-By: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
MrGVSV
left a comment
There was a problem hiding this comment.
Awesome work! Can't wait to have reflection on my smart mirror!
|
Hey, I've never touched Bevy before but I do have a strong opinion against spin locks. Would it be better to use |
That looks perfect honestly, thanks for the advice! I'll update this PR to use |
|
Ok @GnomedDev I've spent a good couple of hours working with
Point 1 could be addressed by just accepting that we lose the concurrency boost an
The problem being if a write lock is requested between steps 1 and 3 (remembering that this time-frame can be large, since multiple readers are valid), the CS isn't actually held, so the writing thread would acquire the CS, see that the lock is in use, and then have no way to park execution, other than just spinning. To confirm this belief, I attempted to find a pre-made So, where to from here? In my opinion, a |
Head branch was pushed to by a user without write access
# Objective - Contributes to bevyengine#15460 ## Solution - Added `std` feature (enabled by default) ## Testing - CI - `cargo check -p bevy_reflect --no-default-features --target "x86_64-unknown-none"` - UEFI demo application runs with this branch of `bevy_reflect`, allowing `derive(Reflect)` ## Notes - The [`spin`](https://crates.io/crates/spin) crate has been included to provide `RwLock` and `Once` (as an alternative to `OnceLock`) when the `std` feature is not enabled. Another alternative may be more desirable, please provide feedback if you have a strong opinion here! - Certain items (`Box`, `String`, `ToString`) provided by `alloc` have been added to `__macro_exports` as a way to avoid `alloc` vs `std` namespacing. I'm personally quite annoyed that we can't rely on `alloc` as a crate name in `std` environments within macros. I'd love an alternative to my approach here, but I suspect it's the least-bad option. - I would've liked to have an `alloc` feature (for allocation-free `bevy_reflect`), unfortunately, `erased_serde` unconditionally requires access to `Box`. Maybe one day we could design around this, but for now it just means `bevy_reflect` requires `alloc`. --------- Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Objective
no_stdBevy #15460Solution
stdfeature (enabled by default)Testing
cargo check -p bevy_reflect --no-default-features --target "x86_64-unknown-none"bevy_reflect, allowingderive(Reflect)Notes
spincrate has been included to provideRwLockandOnce(as an alternative toOnceLock) when thestdfeature is not enabled. Another alternative may be more desirable, please provide feedback if you have a strong opinion here!Box,String,ToString) provided byallochave been added to__macro_exportsas a way to avoidallocvsstdnamespacing. I'm personally quite annoyed that we can't rely onallocas a crate name instdenvironments within macros. I'd love an alternative to my approach here, but I suspect it's the least-bad option.allocfeature (for allocation-freebevy_reflect), unfortunately,erased_serdeunconditionally requires access toBox. Maybe one day we could design around this, but for now it just meansbevy_reflectrequiresalloc.