chore: Adapt ThreadIdManager for MSRV 1.80#91
Open
polarathene wants to merge 2 commits intoAmanieu:masterfrom
Open
chore: Adapt ThreadIdManager for MSRV 1.80#91polarathene wants to merge 2 commits intoAmanieu:masterfrom
ThreadIdManager for MSRV 1.80#91polarathene wants to merge 2 commits intoAmanieu:masterfrom
Conversation
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 free
to 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.
UPDATE: There's an alternative that wouldn't bump the MSRV if adding another crate were acceptable.
Alternative details - Collapsed for brevity
The MSRV bump could be avoided if choosing to use the
BinaryHeapfrom thedary_heapcrate (with theextrasfeature forconst fnsupport), that has an MSRV of 1.61 which is already within the MSRV ofthread_local:Beyond
std::collections::BinaryHeap, the only otherstdimport that couldn't be switched over tocoreisstd::sync::Mutex. Adopting a crate likemcslockorspinfor a spin-lock instead of OS Mutex would enableno_stdcompatibility forthread_local👍 (granted spin locks aren't ideal, nor is it desired withinthread_localif it can be avoided)#58 also contributed a custom minimal spinlock
Mutexthat looks to be adaptable tono_stdcompatibility too.Change Proposed
This reverts changes from #76 related to workaround
BinaryHeap::new()previously lackingconst fnsupport which arrived in Rust1.80.0.OnceLock(raising the MSRV to Rust1.70.0).static Muteximplementation from Remove once_cell dependency #76 , but raises the MSRV to1.80.0whereBinaryHeap::new()isconst fncompatible.The two separate commits was for convenience if preferring to be more conservative towards the MSRV bump. Based on the previous MSRV bump, it does seem acceptable to merge this PR without staging it out into separate PRs/releases that incrementally raise the MSRV.
18 month MSRV:
1.63.0(August 2022) and was merged in April 2024.1.70.0(June 2024)1.80.0(July 2024) both fall within this acceptable MSRV bump range (or rather will by end of Feb 2026).NOTE: Probably should delay merging this until #83 has been merged/released?
There is not much incentive for this change beyond simplifying the code a little bit with the
const fnsupport. I have not checked if it makes any meaningful performance difference.As for concerns raised in #50 (comment)
1.58.0(Jan 2022) at the time (April 2023), with the1.63(Aug 2023) MSRV bump being accepted by Remove once_cell dependency #76 in April 2024.1.10.0release of this crate, there is presumably low churn that it's fine to raise MSRV (as explained by earlier observations), and that the Cargo resolver supportsrust-versionsince Rust1.56.0? So those on toolchains of Rust prior to1.80.0(July 2024) would still get the1.10.0crate release.