From 8a406358af2fe8af1e7f9508ef89d4ec90f23f8e Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 8 Mar 2024 14:06:49 +0900 Subject: [PATCH] index: migrate RevWalkRevset to be based off new RevWalk trait "for<'index> RevWalk, .." works as of now, but it won't be composed well. So I'll turn CompositeIndex<'_> into &CompositeIndex in the next batch, and remove "for<'index>". --- lib/src/default_index/rev_walk.rs | 19 ++++ lib/src/default_index/revset_engine.rs | 122 +++++++++---------------- 2 files changed, 61 insertions(+), 80 deletions(-) diff --git a/lib/src/default_index/rev_walk.rs b/lib/src/default_index/rev_walk.rs index 3945de64a4..47ac17d242 100644 --- a/lib/src/default_index/rev_walk.rs +++ b/lib/src/default_index/rev_walk.rs @@ -35,6 +35,18 @@ pub(super) trait RevWalk { /// Returns `None` when the iteration is finished. Once `None` is returned, /// this will never resume. In other words, a `RevWalk` is fused. fn next(&mut self, index: &I) -> Option; + + // The following methods are provided for convenience. They are not supposed + // to be reimplemented. + + /// Reattaches the underlying `index`. + fn attach(self, index: I) -> RevWalkBorrowedIndexIter + where + Self: Sized, + I: Sized, + { + RevWalkBorrowedIndexIter { index, walk: self } + } } /// Adapter that turns `RevWalk` into `Iterator` by attaching borrowed `index`. @@ -46,6 +58,13 @@ pub(super) struct RevWalkBorrowedIndexIter { walk: W, } +impl RevWalkBorrowedIndexIter { + /// Turns into `'static`-lifetime walk object by detaching the index. + pub fn detach(self) -> W { + self.walk + } +} + impl> Iterator for RevWalkBorrowedIndexIter { type Item = W::Item; diff --git a/lib/src/default_index/revset_engine.rs b/lib/src/default_index/revset_engine.rs index 4562fc996b..1f655bbbb5 100644 --- a/lib/src/default_index/revset_engine.rs +++ b/lib/src/default_index/revset_engine.rs @@ -23,7 +23,7 @@ use std::sync::Arc; use itertools::Itertools; -use super::rev_walk::RevWalkBuilder; +use super::rev_walk::{RevWalk, RevWalkBuilder}; use super::revset_graph_iterator::RevsetGraphIterator; use crate::backend::{ChangeId, CommitId, MillisSinceEpoch}; use crate::default_index::{AsCompositeIndex, CompositeIndex, IndexEntry, IndexPosition}; @@ -214,51 +214,33 @@ impl ToPredicateFn for EagerRevset { } } -struct RevWalkRevset { - walk: F, +struct RevWalkRevset { + walk: W, } -impl RevWalkRevset -where - // Returns trait object because we can't express the following constraints - // without using named lifetime and type parameter: - // - // for<'index> - // F: Fn(CompositeIndex<'index>) -> _, - // F::Output: Iterator + 'index - // - // There's a workaround, but it doesn't help infer closure types. - // https://github.com/rust-lang/rust/issues/47815 - // https://users.rust-lang.org/t/hrtb-on-multiple-generics/34255 - F: Fn(CompositeIndex<'_>) -> Box + '_>, -{ - fn new(walk: F) -> Self { - RevWalkRevset { walk } - } -} - -impl fmt::Debug for RevWalkRevset { +impl fmt::Debug for RevWalkRevset { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("RevWalkRevset").finish_non_exhaustive() } } -impl InternalRevset for RevWalkRevset +impl InternalRevset for RevWalkRevset where - F: Fn(CompositeIndex<'_>) -> Box + '_>, + W: for<'index> RevWalk, Item = IndexPosition> + Clone, { fn entries<'a, 'index: 'a>( &'a self, index: CompositeIndex<'index>, ) -> Box> + 'a> { - Box::new((self.walk)(index).map(move |pos| index.entry_by_pos(pos))) + let positions = self.walk.clone().attach(index); + Box::new(positions.map(move |pos| index.entry_by_pos(pos))) } fn positions<'a, 'index: 'a>( &'a self, index: CompositeIndex<'index>, ) -> Box + 'a> { - (self.walk)(index) + Box::new(self.walk.clone().attach(index)) } fn into_predicate<'a>(self: Box) -> Box @@ -269,15 +251,16 @@ where } } -impl ToPredicateFn for RevWalkRevset +impl ToPredicateFn for RevWalkRevset where - F: Fn(CompositeIndex<'_>) -> Box + '_>, + W: for<'index> RevWalk, Item = IndexPosition> + Clone, { fn to_predicate_fn<'a, 'index: 'a>( &'a self, index: CompositeIndex<'index>, ) -> Box) -> bool + 'a> { - predicate_fn_from_positions(self.positions(index)) + let positions = self.walk.clone().attach(index); + predicate_fn_from_positions(positions) } } @@ -736,24 +719,17 @@ impl<'index> EvaluationContext<'index> { } ResolvedExpression::Ancestors { heads, generation } => { let head_set = self.evaluate(heads)?; - let head_positions = head_set.positions(index).collect_vec(); + let head_positions = head_set.positions(index); + let builder = RevWalkBuilder::new(index).wanted_heads(head_positions); if generation == &GENERATION_RANGE_FULL { - Ok(Box::new(RevWalkRevset::new(move |index| { - Box::new( - RevWalkBuilder::new(index) - .wanted_heads(head_positions.iter().copied()) - .ancestors(), - ) - }))) + let walk = builder.ancestors().detach(); + Ok(Box::new(RevWalkRevset { walk })) } else { let generation = to_u32_generation_range(generation)?; - Ok(Box::new(RevWalkRevset::new(move |index| { - Box::new( - RevWalkBuilder::new(index) - .wanted_heads(head_positions.iter().copied()) - .ancestors_filtered_by_generation(generation.clone()), - ) - }))) + let walk = builder + .ancestors_filtered_by_generation(generation) + .detach(); + Ok(Box::new(RevWalkRevset { walk })) } } ResolvedExpression::Range { @@ -771,27 +747,19 @@ impl<'index> EvaluationContext<'index> { head_set.positions(index), root_positions.iter().copied(), |pos1, pos2| pos1.cmp(pos2).reverse(), - ) - .collect_vec(); + ); + let builder = RevWalkBuilder::new(index) + .wanted_heads(head_positions) + .unwanted_roots(root_positions); if generation == &GENERATION_RANGE_FULL { - Ok(Box::new(RevWalkRevset::new(move |index| { - Box::new( - RevWalkBuilder::new(index) - .wanted_heads(head_positions.iter().copied()) - .unwanted_roots(root_positions.iter().copied()) - .ancestors(), - ) - }))) + let walk = builder.ancestors().detach(); + Ok(Box::new(RevWalkRevset { walk })) } else { let generation = to_u32_generation_range(generation)?; - Ok(Box::new(RevWalkRevset::new(move |index| { - Box::new( - RevWalkBuilder::new(index) - .wanted_heads(head_positions.iter().copied()) - .unwanted_roots(root_positions.iter().copied()) - .ancestors_filtered_by_generation(generation.clone()), - ) - }))) + let walk = builder + .ancestors_filtered_by_generation(generation) + .detach(); + Ok(Box::new(RevWalkRevset { walk })) } } ResolvedExpression::DagRange { @@ -800,23 +768,21 @@ impl<'index> EvaluationContext<'index> { generation_from_roots, } => { let root_set = self.evaluate(roots)?; - let root_positions = root_set.positions(index).collect_vec(); + let root_positions = root_set.positions(index); let head_set = self.evaluate(heads)?; - let head_positions = head_set.positions(index).collect_vec(); + let head_positions = head_set.positions(index); + let builder = RevWalkBuilder::new(index).wanted_heads(head_positions); if generation_from_roots == &(1..2) { - let root_positions_set: HashSet<_> = root_positions.iter().copied().collect(); - let candidates = RevWalkRevset::new(move |index| { - Box::new( - RevWalkBuilder::new(index) - .wanted_heads(head_positions.iter().copied()) - .ancestors_until_roots(root_positions.iter().copied()), - ) - }); + let root_positions: HashSet<_> = root_positions.collect(); + let walk = builder + .ancestors_until_roots(root_positions.iter().copied()) + .detach(); + let candidates = RevWalkRevset { walk }; let predicate = as_pure_predicate_fn(move |_index, entry| { entry .parent_positions() .iter() - .any(|parent_pos| root_positions_set.contains(parent_pos)) + .any(|parent_pos| root_positions.contains(parent_pos)) }); // TODO: Suppose heads include all visible heads, ToPredicateFn version can be // optimized to only test the predicate() @@ -825,18 +791,14 @@ impl<'index> EvaluationContext<'index> { predicate, })) } else if generation_from_roots == &GENERATION_RANGE_FULL { - let mut positions = RevWalkBuilder::new(index) - .wanted_heads(head_positions) - .descendants(root_positions) - .collect_vec(); + let mut positions = builder.descendants(root_positions).collect_vec(); positions.reverse(); Ok(Box::new(EagerRevset { positions })) } else { // For small generation range, it might be better to build a reachable map // with generation bit set, which can be calculated incrementally from roots: // reachable[pos] = (reachable[parent_pos] | ...) << 1 - let mut positions = RevWalkBuilder::new(index) - .wanted_heads(head_positions) + let mut positions = builder .descendants_filtered_by_generation( root_positions, to_u32_generation_range(generation_from_roots)?,