-
Notifications
You must be signed in to change notification settings - Fork 126
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
Make api/rpc interfaces no_std compatible #384
Conversation
@@ -15,11 +15,7 @@ | |||
|
|||
*/ | |||
|
|||
#[cfg(not(feature = "std"))] |
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::string::String
points to alloc::string::String
as well. So I removed this obsolete condition completely.
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.
LGTM
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 looks good!
Just remember, if IoT should be one of the target deployments of this, we might want to get rid of thisError
in no_std whatsoever, at least for production builds. The footprint of the error-chain that errors contain is not negligible.
primitives/src/rpc_params.rs
Outdated
#[cfg(not(feature = "std"))] | ||
/// Insert a plain value into the builder with heap allocation. If available, | ||
/// use the more efficient std version. | ||
pub(crate) fn insert<P: Serialize>(&mut self, value: P) -> Result<()> { | ||
self.insert_with_allocation(value) | ||
} |
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.
Usually the doc is above the feature flag
Good point. But maybe a full cleanup would be sensible, as propagating Strings is also not very efficient, I suppose. I commented in #317 accordingly. |
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.
LGTM!
One step further towards
no_std
compatibility:compsoe-macros
is now fullyno_std
compatibleapi/rpc
is now fullyno_std
compatibleprimitives-types
import inCargo.toml
Drawbacks:
thiserror
needed to be changed tothiserror-core
. But that should be temporary only. And better than removing it completely (I hope).no_std
.one step towards #279