Fix TODO items #166
Draft
Fix TODO items #166
Conversation
This file contains 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
`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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.