Fix TODO items - #166
Draft
8573 wants to merge 2 commits into
Draft
Conversation
`tinyvec` currently uses crate features `rustc_1_40`, `rustc_1_55`, and `rustc_1_57` to enable functions and optimizations that require Rust versions higher than the library's base MSRV of 1.34.0. This patch replaces the uses of these crate features with dtolnay's macro `rustversion`, which automatically detects the version of `rustc` with which the code is being compiled, thus allowing the optimizations enabled by newer Rust versions to be applied regardless of whether a dependent requests any of the `rustc_*` crate features. This may be especially useful for dependents whose own MSRVs would bar them from using those crate features without gating them behind crate features of their own, potentially requiring a proliferation of such crate features through a dependency tree. I would have limited this patch to using `rustversion` to enable optimizations automatically and not to replace the use of the crate features to gate functions that require Rust versions newer than 1.34.0, because I thought that using `rustversion` rather than the crate features to gate such functions might have been undesirable because it would mean losing the "Available on crate feature xyz only" hints on Docs.rs, but I see that Docs.rs doesn't apply those hints to functions anyway, so no such hints are lost by switching the gating mechanism to `rustversion`. This patch further uses `rustversion` to add compilation errors in case one of the `rustc_*` crate features is requested and the available Rust version is too old, such that the `rustc_*` crate features now function simply as static assertions that the running `rustc` supports the indicated Rust version. This patch, of course, adds a dependency on `rustversion`, which becomes this library's only non-optional dependency. Its MSRV is 1.31.0 and so does not raise the MSRV of this library. If having a non-optional dependency is unacceptable, an alternative could be to have `rustversion` be an optional, on-by-default dependency and to rely on the `rustc_*` crate features as before if `rustversion` is disabled. Rather than #[rustversion::since(1.57)] the conditional compilation clauses would look like #[cfg(any(feature = "rustversion", feature = "rustc_1_57"))] #[cfg_attr(feature = "rustversion", rustversion::since(1.57))] which is verbose enough that I suspect that rejecting `rustversion` altogether would be preferred. I admit that I do not understand why the comment in `Cargo.toml` on the crate feature `rustc_1_40` seems to say that "us[ing] Vec::append if possible in TinyVec::append" and overriding `DoubleEndedIterator::nth_back` require Rust 1.37 and Rust 1.40 respectively, when the standard library documentation says that `Vec::append` and `DoubleEndedIterator::nth_back` were stabilized in Rust 1.4.0 and Rust 1.37.0 respectively.
Implement two things that were tracked in TODO comments, although only one appears to be in live code: using `core::mem::take` rather than a reimplementation of it when Rust 1.40 is available. I changed the `#[inline(always)]` on the reimplemented `take` to `#[inline]` to match what `core` has, figuring that `core` would know best what level of inlining hint it should have. On the other hand, do `tinyvec`'s benchmarks indicate that `#[inline(always)]` helps? If so, I suppose reimplementing the function in all cases may be preferred, in which case this patch would be unnecessary.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement two things that were tracked in TODO comments, although only one appears to be in live code: using
core::mem::takerather than a reimplementation of it when Rust 1.40 is available.I changed the
#[inline(always)]on the reimplementedtaketo#[inline]to match whatcorehas, figuring thatcorewould know best what level of inlining hint it should have. On the other hand, dotinyvec's benchmarks indicate that#[inline(always)]helps? If so, I suppose reimplementing the function in all cases may be preferred, in which case this patch would be unnecessary.This patch, as it is currently written, depends on #165 and is marked as a draft to prevent it from being accidentally merged before #165. If you merge #165, feel free to merge this too regardless of its "draft" state. If you want this patch but not #165, it should not be difficult to rewrite this patch for that case instead.