Skip to content

Commit 41fc046

Browse files
committed
Address review comments
1 parent adc8828 commit 41fc046

File tree

1 file changed

+62
-6
lines changed

1 file changed

+62
-6
lines changed

src/imports.rs

Lines changed: 62 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,7 @@ fn condense_use_trees(use_trees: &mut Vec<UseTree>) {
283283
continue;
284284
}
285285
if curr_len < 1
286-
|| !(curr_tree.is_singleton() || curr_len + 1 == curr_tree.path.len())
286+
|| !curr_tree.is_singleton() && curr_len + 1 != curr_tree.path.len()
287287
|| curr_tree.attrs.is_some()
288288
|| curr_tree.contains_comment()
289289
|| !curr_tree.same_visibility(singleton)
@@ -726,11 +726,11 @@ impl UseTree {
726726
}
727727

728728
fn shared_prefix_len(&self, other: &UseTree) -> usize {
729-
let mut n = 0;
730-
while n < self.path.len() && n < other.path.len() && self.path[n] == other.path[n] {
731-
n += 1;
732-
}
733-
n
729+
self.path
730+
.iter()
731+
.zip(other.path.iter())
732+
.take_while(|(a, b)| a == b)
733+
.count()
734734
}
735735

736736
fn is_singleton(&self) -> bool {
@@ -1396,6 +1396,62 @@ mod test {
13961396
);
13971397
}
13981398

1399+
#[test]
1400+
fn test_use_tree_merge_module_condensed() {
1401+
test_merge!(
1402+
ModuleCondensed,
1403+
["foo::b", "foo::{a, c, d::e}"],
1404+
["foo::{a, b, c, d::e}"]
1405+
);
1406+
1407+
test_merge!(
1408+
ModuleCondensed,
1409+
["foo::{a::b, a::c, d::e, d::f}"],
1410+
["foo::a::{b, c}", "foo::d::{e, f}"]
1411+
);
1412+
}
1413+
1414+
/// This test exercises the "prefer the earliest one" logic in [`condense_use_trees`]. In
1415+
/// particular, if this line:
1416+
/// ```
1417+
/// for curr_index in 0..n {
1418+
/// ```
1419+
/// were changed to this line:
1420+
/// ```
1421+
/// for curr_index in (0..n).rev() {
1422+
/// ```
1423+
/// the test would fail. An explanation follows.
1424+
///
1425+
/// Note that [`condense_use_trees`]'s outer loop visits the trees back-to-front. So in this
1426+
/// test, `foo::e::f` will be the first singleton considered for merging.
1427+
///
1428+
/// Next note that `foo::a::b` and `foo::c::d` are equally good candidates to absorb
1429+
/// `foo::e::f`. Because the earliest candidate (i.e., `foo::a::b`) is selected, we get the
1430+
/// following intermediate state:
1431+
/// ```
1432+
/// foo::{a::b, e::f};
1433+
/// foo::c::d;
1434+
/// ```
1435+
/// Then, when `foo::c::d` is visited by the outer loop, it will notice that this singleton can
1436+
/// be merged into `foo::{a::b, e::f}`, and we get the desired output.
1437+
///
1438+
/// On the other hand, if `foo::c::d` were selected, we would get the following intermediate
1439+
/// state:
1440+
/// ```
1441+
/// foo::a::b;
1442+
/// foo::{c::d, e::f};
1443+
/// ```
1444+
/// From this state, no progress could be made, because the only singleton (`foo::a::b`) does
1445+
/// not appear after any tree that can absorb it.
1446+
#[test]
1447+
fn test_use_tree_merge_module_condensed_relative_order() {
1448+
test_merge!(
1449+
ModuleCondensed,
1450+
["foo::a::b", "foo::c::d", "foo::e::f"],
1451+
["foo::{a::b, c::d, e::f}"]
1452+
);
1453+
}
1454+
13991455
#[test]
14001456
fn test_use_tree_merge_one() {
14011457
test_merge!(One, ["a", "b"], ["{a, b}"]);

0 commit comments

Comments
 (0)