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

Feature guarded no_std support #1

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

Feature guarded no_std support #1

wants to merge 4 commits into from

Conversation

dzmitry-huba
Copy link
Owner


name: no_std
about: Supporting no_std compilation mode


Description of feature:
Make the library compatible with no_std environments.

Implementation:
The std target implementation remains unchanged. The majority of changes are switching to using core and alloc where std used to be. The no_std flavor uses no_std compatible collections and synchronization primitives. The thiserror crate doesn't have no_std support hence the error types where reimplemented manually.

name: no_std
about: Supporting no_std compilation mode

---

**Description of feature:**
Make the library compatible with no_std environments.

**Implementation:**
The std target implementation remains unchanged. The majority of changes are switching to using core and alloc where std used to be. The no_std flavor uses no_std compatible collections and synchronization primitives. The thiserror crate doesn't have no_std support hence the error types where reimplemented manually.

Signed-off-by: Dzmitry Huba <[email protected]>
Copy link

@tiziano88 tiziano88 left a comment

Choose a reason for hiding this comment

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

It all looks sensible, and the changes were not as invasive as I would have thought


/// The base error type for raft
#[derive(Debug, Error)]
#[derive(Debug)]

Choose a reason for hiding this comment

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

Why was Error removed? Is it not no_std compatible at all? I thought the issue was just with the IO errors.

Perhaps consider one of those instead

pub enum Error {
/// An IO error occurred
#[error("{0}")]
Io(#[from] std::io::Error),
#[cfg(feature = "std")]

Choose a reason for hiding this comment

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

Add a comment explaining why this needs to be excluded in no_std.

@@ -1,61 +1,110 @@
// Copyright 2019 TiKV Project Authors. Licensed under Apache-2.0.
use thiserror::Error;

Choose a reason for hiding this comment

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

After this change is thiserror used at all? Could you get rid of that dependency altogether?

(Or, if it is used somewhere else, why is that "somewhere else" not breaking no_std?)

Signed-off-by: Dzmitry Huba <[email protected]>
…uses stable version and no-std requires an experimental feature.

Signed-off-by: Dzmitry Huba <[email protected]>
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.

3 participants