Skip to content
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

chore: improve no_std maintainability: #1047

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

gibbz00
Copy link
Contributor

@gibbz00 gibbz00 commented Apr 28, 2024

Similar to how rustls does it:

https://github.com/rustls/rustls/blob/513e374b2e2ce9f1fb57ac78ab3ca053afc8f133/rustls/src/lib.rs#L353-L359

... extern crate plus the #![no_std] attribute changes the
default prelude from std::prelude to core::prelude. That forces one to explicitly import (use) everything that is in std::prelude but not in core::prelude. This helps maintain no-std support as even developers that are not interested in, or aware of, no-std support and / or that never run cargo build --no-default-features locally will get errors when they rely on std::prelude API.

@gibbz00 gibbz00 force-pushed the no_std branch 2 times, most recently from c6b53e6 to 4023a9c Compare April 28, 2024 10:00
Similar to how `rustls` does it:

https://github.com/rustls/rustls/blob/513e374b2e2ce9f1fb57ac78ab3ca053afc8f133/rustls/src/lib.rs#L353-L359

> ... `extern crate` plus the `#![no_std]` attribute changes the
default prelude from `std::prelude` to `core::prelude`. That forces
one to _explicitly_ import (`use`) everything that is in `std::prelude`
but not in `core::prelude`. This helps maintain no-std support as even
developers that are not interested in, or aware of, no-std support and /
or that never run `cargo build --no-default-features` locally will get
errors when they rely on `std::prelude` API.
@gibbz00 gibbz00 marked this pull request as draft April 28, 2024 17:31
Makes it easier to fix tokio-rs#939 because there were places where the
prost_path setting was being ignored when direct a direct alloc path
would suffice:

https://github.com/tokio-rs/prost/blob/691454307ccd2de4c780438da8acf2a1ddd994f6/prost-build/src/code_generator.rs#L648
Copy link
Collaborator

@caspermeijn caspermeijn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of better no_std maintainability, but this PR introduces multiple concepts. I think it would be better reviewable if those are split into multiple PRs.

pub enum Data {
#[prost(message, tag="1")]
Foo(::prost::alloc::boxed::Box<super::Foo>),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the generated code should always use a fully qualified type path, as they could otherwise clash with a user-generated type.

@@ -0,0 +1,90 @@
//! A facade around all the types we need from the `std`, `core`, and `alloc`
//! crates. This avoids elaborate import wrangling having to happen in every
//! module.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the facade concept. If we keep it, it should work like the ::std::prelude. But I think it is better to be explicit about your dependencies.

#![doc = include_str!("../../README.md")]
#![no_std]

// See: https://github.com/tokio-rs/prost/pull/1047
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code should not refer to Github issues, but instead contain the contents directly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants