-
Notifications
You must be signed in to change notification settings - Fork 56
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
Add no_std
support
#265
base: main
Are you sure you want to change the base?
Add no_std
support
#265
Conversation
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.
Nice to see gpu-allocator
usable on no_std
with relatively minor changes! Thanks for doing the setup work here.
Just not sure if we're happy to commit to this rather tricky list of mutually exclusive features here. Consider someone uses gpu-allocator
in their library crate and sets default-features = false
to make it compile with a minimal set and no_std
-compliance by default. They now have to enable the hashbrown
feature to get it to compile.
When some other application crate uses that library crate, and also uses gpu-allocator
with std
feature to get backtrace
support, this will now no longer compile because those are mutually exclusive.
I've suggested a change to make the hashbrown
and std
features coexist.
Can you add these lints to ensure this'll be easier to maintain in the future:
#![warn(
clippy::alloc_instead_of_core,
clippy::std_instead_of_alloc,
clippy::std_instead_of_core,
)]
And update the CI to ensure that it tests both no_std
as well as std
: we can add these to the matrix
in the check_msrv
, test
and clippy
jobs.
Finally, I ended up not spending any comments on it but instead recommend you to undo a few unnecessary import reorderings with:
cargo fmt -- --config imports_granularity=Crate --config group_imports=StdExternalCrate
The output isn't ideal (you have to group visualizer
mod
+use
back together for example) but at least the ordering is back in check: log
goes separate from the std
/core
/alloc
"standard libraries" for example.
Cargo.toml
Outdated
thiserror = "1.0" | ||
presser = { version = "0.3" } | ||
log = { version = "0.4", default-feature = false } | ||
thiserror = { version = "2.0", default-feature = false } |
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.
Keep thiserror
at 1
for now instead of pushing this drive-by change?
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.
std
can be disabled in thiserror
since v2, but its msrv is 1.81+ dtolnay/thiserror#373.
But we can use derive_more instead, its msrv is 1.75+.
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.
@MarijnS95
Should we use derive_more for bumping lower MSRV?
Almost forgot to mention: can you please drop the
I don't think a one-off |
a22352a
to
eb9e04c
Compare
|
…e `free_list_allocator` by feature
c5b70f0
to
f2a6bfb
Compare
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.
Nice update!
Only two things remaining: the formatting of the reports is broken, please test your changes to these before pushing them. I've suggested how to fix this in one file.
The previous review contained a request to update the CI to ensure that we test both with and without these new feature combinations, that would be very helpful to prevent breakage going forward. Right now we always build with specific backends and --no-default-features
, meaning neither "hashbrown"
nor "std"
(nor a combination) ever gets exercised properly.
No description provided.