-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
no_std
for naga
#7249
no_std
for naga
#7249
Conversation
This is an alternative to #6940 (special thanks to @brody4hire for working on this while I was busy doing other things!) I believe this PR is in a more polished state as-is and ready to merge. There are no outstanding todo items, and the only dependency added is One possible downside to consider is It was also raised by @kpreid that the use of an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have some high level concerns and some notes
- The
libm
andcodespan-reporting
changes should be split into 2 separate PRs, both from a reviewability perspective, and ensuring we can pinpoint any regressions.
use alloc::borrow::Cow; | ||
use alloc::format; | ||
use alloc::string::String; | ||
use alloc::string::ToString; | ||
use alloc::vec::Vec; | ||
use core::fmt::{Error as FmtError, Write as _}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should remain as compacted as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using traits, which require this very kludgy fully-qualified syntax on calling. Instead I think the better plan would be making two inline modules with wrapper methods, then conditionally compiling each module and re-exporting as appropriate. Overloads can be dealt with suffixes. That way we can call math::sin_f32
and such directly.
As noted above though, this change needs to be in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use a trait with methods? It even already exists, and this is how I handle the problem:
#[cfg(not(feature = "std"))]
use num_traits::float::Float as _;
With the libm
feature enabled, Float
is a nearly perfect polyfill for std
float methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would work!
use super::ValidationError; | ||
use crate::non_max_u32::NonMaxU32; | ||
use crate::{ | ||
arena::{BadHandle, BadRangeError}, | ||
diagnostic_filter::DiagnosticFilterNode, | ||
Handle, | ||
}; | ||
|
||
use crate::non_max_u32::NonMaxU32; | ||
use crate::{Arena, UniqueArena}; | ||
use core::{convert::TryInto, hash::Hash}; | ||
|
||
use super::ValidationError; | ||
|
||
use std::{convert::TryInto, hash::Hash}; | ||
#[cfg(test)] | ||
use alloc::string::ToString; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we don't follow this super strictly, but we should try to follow rustfmt's StdExternalCrate ordering for imports. You don't need to fix things you didn't touch, but when changing them, we should try to standardize them.
# Check with all features except "std". | ||
cargo clippy --target ${{ matrix.target }} ${{ matrix.extra-flags }} -p wgpu-types --no-default-features --features strict_asserts,fragile-send-sync-non-atomic-wasm,serde,counters | ||
cargo clippy --target ${{ matrix.target }} ${{ matrix.extra-flags }} -p naga --no-default-features --features libm,dot-out,glsl-out,msl-out,msl-out-if-target-apple,wgsl-out,hlsl-out,hlsl-out-if-target-windows,compact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have concerns about this list atrophying, I'm not sure what should be done about that though.
## Provide rich error messages using codespan-reporting | ||
codespan-reporting = ["std", "dep:codespan-reporting", "dep:termcolor"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry that cordoning codespan-reporting off completely will mean that wgpu-core
won't be able to format errors like it expects to, as it definitely pretty prints the errors there.
I think these are really good points and very actionable! I'm going to close this PR in favour of the first and least controversial part, which is simply importing from |
Connections
Description
std
,libm
, andcodespan-reporting
features.std
to features which are incompatible withno_std
to document compatibilitycore
andalloc
instead ofstd
where requirednaga
to the existingno_std
CI checkTesting
cargo check -p naga --no-default-features --target=wasm32v1-none --features libm,dot-out,glsl-out,msl-out,msl-out-if-target-apple,wgsl-out,hlsl-out,hlsl-out-if-target-windows,compact
Squash or Rebase?
Squash before merging.
Checklist
cargo fmt
.taplo format
.cargo clippy
. If applicable, add:--target wasm32-unknown-unknown
cargo xtask test
to run tests.CHANGELOG.md
. See simple instructions inside file.Notes for Reviewers
naga/src/math.rs
which just handles bringing in maths operations from eitherstd
orlibm
depending on what features are enabled. The rest of the changes are mostly changing import names and sprinkling a few#[cfg(feature = "...")]
statements.Feature Compatibility
std
(e.g.,x86_64-pc-windows-msvc
for Windows)no_std
(e.g.,wasm32v1-none
)default
std
libm
dot-out
glsl-out
msl-out
msl-out-if-target-apple
wgsl-out
hlsl-out
hlsl-out-if-target-windows
compact
glsl-in
serialize
deserialize
arbitrary
spv-in
spv-out
wgsl-in
codespan-reporting
Note that many of these features could be made to be compatible with
no_std
, but since they are optional I am leaving them for a follow-up to reduce the size of this PR as much as possible.