Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

remove no-std-compat #43

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Luro02
Copy link

@Luro02 Luro02 commented Sep 17, 2020

This PR removes the dependency on the no-std-compat crate to make this crate have zero dependencies.

There is little benefit in using the crate, because as can be seen in the diff of the PR, there are only a few lines that have to be changed to make it work without it.

On my local machine the tests failed on nightly with confusing errors like this one:

error: #[quickcheck] is only supported on statics and functions
    --> src/tests.rs:1162:9
     |
1162 |         fn reordering_compact(insertions: u16, to_delete: Vec<u16>) -> bool {
     |         ^^
...
1238 |     gen_tests_for!(InlineStableVec);
     |     -------------------------------- in this macro invocation

idk why they fail, but they work on stable.

@LukasKalbertodt
Copy link
Owner

Thanks for the PR. Regarding the strange error: that should be gone after rebasing on master.

However, I'm not sure about this change. What exactly is the practical benefit? The no-std-compat crate compiles extremely fast as it's just a bunch of reexports. And I do prefer using std everywhere instead of mixing core and alloc imports. Have you seen #36 which adds this dependency?

@Luro02
Copy link
Author

Luro02 commented Oct 17, 2020

What exactly is the practical benefit?

Making the dependency tree smaller.

Having less (redundant) dependencies is always better, because less code has to be compiled, documented and downloaded (even if it is just a few kilobytes).

I think this is a very debatable topic and really reminds me of the discussions revolving around the std library and that it should provide more functionality like the C++ standard library does. example thread (there are quite a few of them)

I do not mind crates having dependencies, but for example if a crate depends on thiserror for a single error type like this one:

use thiserror::Error;

#[derive(Error, Debug, Clone)]
pub enum DataStoreError {
    #[error("data store disconnected")]
    Disconnect(#[from] io::Error),
    #[error("the data for key `{0}` is not available")]
    Redaction(String),
    #[error("invalid header (expected {expected:?}, found {found:?})")]
    InvalidHeader {
        expected: String,
        found: String,
    },
    #[error("unknown data store error")]
    Unknown,
}

I would make a PR to remove thiserror as there is not much of a benefit in using it.

I feel similarly about no-std-compat, which makes sense to depend on, if you have a large crate, that you want to make no-std compatible, without rewriting hundreds of imports, but this crate only has very few imports, so it is not that much work to rewrite them with core and alloc.

And I do prefer using std everywhere instead of mixing core and alloc imports

I would actually prefer to use core/alloc everywhere and only use std for when it is absolutely necessary.

Cargo features are designed to be opt-in, which is why you should do something like this:

[features]
default-features = ["std"]
std = []

instead of

[features]
default-features = []
no-std = []

Using std for the entire crate just does not feel right, because according to the features std should be optional. (So you only import std, if the feature is enabled and everything else imports core/alloc)

It also makes the code more readable as you can clearly see which imports make allocations and which do not.

The no-std-compat crate compiles extremely fast as it's just a bunch of reexports

Yes, but is it really necessary to compile this? It feels like an unnecessary dependency (especially for a crate with so few imports).

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

Successfully merging this pull request may close these issues.

2 participants