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

Refactor: tidy up imports and features #26

Merged
merged 4 commits into from
May 4, 2023
Merged

Refactor: tidy up imports and features #26

merged 4 commits into from
May 4, 2023

Conversation

BastiDood
Copy link
Contributor

@BastiDood BastiDood commented Aug 14, 2022

Hello there! Just wanted to send in a PR that cleans up the feature-gated imports for this crate. To make the review easier, I made sure to neatly separate each commit according to their purpose.

std implies alloc

The first major change I did was to adjust the feature list so that std now implies alloc. That way, we no longer have to check the condition any(feature = "std", feature = "alloc") anymore.

[features]
default = ["std"]
alloc = []
std = ["alloc"] # `std` implies `alloc`

As much as possible, I imported from core. I only imported from the feature-gated alloc and std crates when absolutely necessary (and as a last resort).

std not necessary?

After restructuring the imports and features, I noticed that many of the imported items were actually unused. Namely:

  • alloc::collections::{BTreeMap, BTreeSet}
  • core::hash::{BuildHasher, Hash}
  • std::collections::{HashMap, HashSet}

In fact, the only item from alloc that the crate directly uses is alloc::boxed::Box.1 All other imports from alloc and std are unused. I have thus taken the liberty in removing them.

But, this does introduce an interesting situation: the std feature is actually unused now! We can completely remove it from the crate's exposed features.

I'm not exactly sure why this is the case. The documentation mentions that the crate:

... provides implementations for Box, Vec, BTreeMap, and BTreeSet [when alloc is enabled]. If the std feature is enabled, this crate additionally provides implementations for HashMap and HashSet.

Nevertheless, I've removed the std feature because it is indeed unused. Implementing the missing features is beyond the scope of this PR. Future PRs may want to consider upholding that std implies alloc when std is re-introduced into the crate.

Fixing some Clippy lints...

When the errors finally disappeared, I noticed a bunch of Clippy warnings. These have been addressed as well. Please see the aptly named commit message for the (rather mechanical) changes.

Fixing some outdated doc-comments...

One of the doc-comments uses the now-removed FromFallibleIterator trait, which causes the example tests to fail. I've resolved this issue.

Bumping the MSRV to 1.36

See #26 (comment).

Footnotes

  1. The Box import was used to impl the FallibleIterator trait for boxed iterators (i.e. Box<I: FallibleIterator>.

@BastiDood
Copy link
Contributor Author

Quick note: the CI error on stable appears to be due to the fact that the current MSRV is below 1.36. Hence, extern crate alloc syntax had not yet been stabilized (see rust-lang/rust#27783).

Of course, if we bump the MSRV to 1.36, then CI should now pass.1 Since 1.36 is already three years old as of writing, I believe that this is a fair breaking change.

Footnotes

  1. Indeed, we can see that the nightly build passes.

@dpc
Copy link
Collaborator

dpc commented Aug 14, 2022

Hi! Thanks for this PR. All LGTM. However I am sort of a new backup maintainer, so I'll let this sit for a couple of days to give a chance for other's to look a this in. If I forget about this PR, please keep @ me in few days until I get it landed.

@tamird
Copy link
Contributor

tamird commented May 4, 2023

@dpc should this land? Looks to be in good shape.

@BastiDood
Copy link
Contributor Author

Thanks for the bump! I completely forgot this was in my backlog. 😅

I've rebased the branch on top of the latest master to further ease the merge.

@dpc dpc merged commit 92e5d59 into sfackler:master May 4, 2023
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