Skip to content

Commit 89e52e2

Browse files
matthewjaspermatthewjasper
committed
Address review comments
1 parent 5f90dbd commit 89e52e2

File tree

2 files changed

+143
-96
lines changed

2 files changed

+143
-96
lines changed

‎src/librustc_mir_build/build/matches/mod.rs

Lines changed: 140 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -153,18 +153,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
153153
arms.iter()
154154
.map(|arm| {
155155
let arm_has_guard = arm.guard.is_some();
156-
let arm_candidate = Candidate {
157-
span: arm.pattern.span,
158-
match_pairs: smallvec![MatchPair::new(*scrutinee, &arm.pattern),],
159-
bindings: vec![],
160-
ascriptions: vec![],
161-
has_guard: arm_has_guard,
162-
needs_otherwise_block: arm_has_guard,
163-
otherwise_block: None,
164-
pre_binding_block: None,
165-
next_candidate_pre_binding_block: None,
166-
subcandidates: vec![],
167-
};
156+
let arm_candidate = Candidate::new(*scrutinee, &arm.pattern, arm_has_guard);
168157
(arm, arm_candidate)
169158
})
170159
.collect()
@@ -195,10 +184,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
195184
self.match_candidates(scrutinee_span, block, &mut otherwise, candidates, &mut fake_borrows);
196185

197186
if let Some(otherwise_block) = otherwise {
187+
// See the doc comment on `match_candidates` for why we may have an
188+
// otherwise block. Match checking will ensure this is actually
189+
// unreachable.
198190
let source_info = self.source_info(scrutinee_span);
199191
self.cfg.terminate(otherwise_block, source_info, TerminatorKind::Unreachable);
200192
}
201193

194+
// Link each leaf candidate to the `pre_binding_block` of the next one.
202195
let mut previous_candidate: Option<&mut Candidate<'_, '_>> = None;
203196

204197
for candidate in candidates {
@@ -449,29 +442,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
449442
initializer: &Place<'tcx>,
450443
set_match_place: bool,
451444
) -> BlockAnd<()> {
452-
// create a dummy candidate
453-
let mut candidate = Candidate {
454-
span: irrefutable_pat.span,
455-
has_guard: false,
456-
needs_otherwise_block: false,
457-
match_pairs: smallvec![MatchPair::new(*initializer, &irrefutable_pat)],
458-
bindings: vec![],
459-
ascriptions: vec![],
460-
461-
// since we don't call `match_candidates`, next fields are unused
462-
otherwise_block: None,
463-
pre_binding_block: None,
464-
next_candidate_pre_binding_block: None,
465-
subcandidates: vec![],
466-
};
445+
let mut candidate = Candidate::new(*initializer, &irrefutable_pat, false);
467446

468447
let fake_borrow_temps =
469448
self.lower_match_tree(block, irrefutable_pat.span, false, &mut [&mut candidate]);
470449

471-
// for matches and function arguments, the place that is being matched
450+
// For matches and function arguments, the place that is being matched
472451
// can be set when creating the variables. But the place for
473452
// let PATTERN = ... might not even exist until we do the assignment.
474-
// so we set it here instead
453+
// so we set it here instead.
475454
if set_match_place {
476455
let mut candidate_ref = &candidate;
477456
while let Some(next) = {
@@ -487,6 +466,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
487466
bug!("Let binding to non-user variable.")
488467
}
489468
}
469+
// All of the subcandidates should bind the same locals, so we
470+
// only visit the first one.
490471
candidate_ref.subcandidates.get(0)
491472
} {
492473
candidate_ref = next;
@@ -666,10 +647,6 @@ struct Candidate<'pat, 'tcx> {
666647
/// This `Candidate` has a guard.
667648
has_guard: bool,
668649

669-
/// This `Candidate` needs and otherwise block, either because it has a
670-
/// guard or it has subcandidates.
671-
needs_otherwise_block: bool,
672-
673650
/// All of these must be satisfied...
674651
match_pairs: SmallVec<[MatchPair<'pat, 'tcx>; 1]>,
675652

@@ -690,7 +667,21 @@ struct Candidate<'pat, 'tcx> {
690667
next_candidate_pre_binding_block: Option<BasicBlock>,
691668
}
692669

693-
impl Candidate<'_, '_> {
670+
impl<'tcx, 'pat> Candidate<'pat, 'tcx> {
671+
fn new(place: Place<'tcx>, pattern: &'pat Pat<'tcx>, has_guard: bool) -> Self {
672+
Candidate {
673+
span: pattern.span,
674+
has_guard,
675+
match_pairs: smallvec![MatchPair { place, pattern }],
676+
bindings: Vec::new(),
677+
ascriptions: Vec::new(),
678+
subcandidates: Vec::new(),
679+
otherwise_block: None,
680+
pre_binding_block: None,
681+
next_candidate_pre_binding_block: None,
682+
}
683+
}
684+
694685
/// Visit the leaf candidates (those with no subcandidates) contained in
695686
/// this candidate.
696687
fn visit_leaves<'a>(&'a mut self, mut visit_leaf: impl FnMut(&'a mut Self)) {
@@ -839,6 +830,21 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
839830
///
840831
/// If `fake_borrows` is Some, then places which need fake borrows
841832
/// will be added to it.
833+
///
834+
/// For an example of a case where we set `otherwise_block`, even for an
835+
/// exhaustive match consider:
836+
///
837+
/// match x {
838+
/// (true, true) => (),
839+
/// (_, false) => (),
840+
/// (false, true) => (),
841+
/// }
842+
///
843+
/// For this match we check if `x.0` matches `true` (for the first
844+
/// arm) if that's false we check `x.1`, if it's `true` we check if
845+
/// `x.0` matches `false` (for the third arm). In the (impossible at
846+
/// runtime) case when `x.0` is now `true` we branch to
847+
/// `otherwise_block`.
842848
fn match_candidates<'pat>(
843849
&mut self,
844850
span: Span,
@@ -1009,7 +1015,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
10091015

10101016
let fully_matched_with_guard = matched_candidates
10111017
.iter()
1012-
.position(|c| !c.needs_otherwise_block)
1018+
.position(|c| !c.has_guard)
10131019
.unwrap_or(matched_candidates.len() - 1);
10141020

10151021
let (reachable_candidates, unreachable_candidates) =
@@ -1021,7 +1027,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
10211027
assert!(candidate.otherwise_block.is_none());
10221028
assert!(candidate.pre_binding_block.is_none());
10231029
candidate.pre_binding_block = Some(next_prebinding);
1024-
if candidate.needs_otherwise_block {
1030+
if candidate.has_guard {
1031+
// Create the otherwise block for this candidate, which is the
1032+
// pre-binding block for the next candidate.
10251033
next_prebinding = self.cfg.start_new_block();
10261034
candidate.otherwise_block = Some(next_prebinding);
10271035
}
@@ -1039,6 +1047,53 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
10391047
reachable_candidates.last_mut().unwrap().otherwise_block
10401048
}
10411049

1050+
/// Tests a candidate where there are only or-patterns left to test, or
1051+
/// forwards to [Builder::test_candidates].
1052+
///
1053+
/// Given a pattern `(P | Q, R | S)` we (in principle) generate a CFG like
1054+
/// so
1055+
///
1056+
/// ```text
1057+
/// [ start ]
1058+
/// |
1059+
/// [ match P, Q ]
1060+
/// |
1061+
/// +----------------------------------------+------------------------------------+
1062+
/// | | |
1063+
/// [ P matches ] [ Q matches ] [ otherwise ]
1064+
/// | | |
1065+
/// [ match R, S ] [ match R, S ] |
1066+
/// | | |
1067+
/// +--------------+------------+ +--------------+------------+ |
1068+
/// | | | | | | |
1069+
/// [ R matches ] [ S matches ] [otherwise ] [ R matches ] [ S matches ] [otherwise ] |
1070+
/// | | | | | | |
1071+
/// +--------------+------------|------------+--------------+ | |
1072+
/// | | | |
1073+
/// | +----------------------------------------+--------+
1074+
/// | |
1075+
/// [ Success ] [ Failure ]
1076+
/// ```
1077+
///
1078+
/// In practice there are some complications:
1079+
///
1080+
/// * If there's a guard, then the otherwise branch of the first match on
1081+
/// `R | S` goes to a test for whether `Q` matches.
1082+
/// * If neither `P` or `Q` has any bindings or type ascriptions and there
1083+
/// isn't a match guard, then we create a smaller CFG like:
1084+
///
1085+
/// ```text
1086+
/// ...
1087+
/// +---------------+------------+
1088+
/// | | |
1089+
/// [ P matches ] [ Q matches ] [ otherwise ]
1090+
/// | | |
1091+
/// +---------------+ |
1092+
/// | ...
1093+
/// [ match R, S ]
1094+
/// |
1095+
/// ...
1096+
/// ```
10421097
fn test_candidates_with_or(
10431098
&mut self,
10441099
span: Span,
@@ -1049,42 +1104,53 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
10491104
) {
10501105
let (first_candidate, remaining_candidates) = candidates.split_first_mut().unwrap();
10511106

1052-
if let PatKind::Or { .. } = *first_candidate.match_pairs[0].pattern.kind {
1053-
let match_pairs = mem::take(&mut first_candidate.match_pairs);
1054-
first_candidate.needs_otherwise_block = true;
1055-
first_candidate.pre_binding_block = Some(block);
1107+
match *first_candidate.match_pairs[0].pattern.kind {
1108+
PatKind::Or { .. } => (),
1109+
_ => {
1110+
self.test_candidates(span, candidates, block, otherwise_block, fake_borrows);
1111+
return;
1112+
}
1113+
}
10561114

1057-
// We sort or-patterns to the end in `simplify_candidate`, so all
1058-
// the remaining match pairs are or-patterns.
1059-
for match_pair in match_pairs {
1060-
if let PatKind::Or { ref pats } = *match_pair.pattern.kind {
1061-
let or_span = match_pair.pattern.span;
1062-
let place = &match_pair.place;
1115+
let match_pairs = mem::take(&mut first_candidate.match_pairs);
1116+
first_candidate.pre_binding_block = Some(block);
10631117

1064-
first_candidate.visit_leaves(|leaf_candidate| {
1065-
self.test_or_pattern(leaf_candidate, pats, or_span, place, fake_borrows);
1066-
});
1067-
} else {
1068-
bug!("Or patterns should have been sorted to the end");
1069-
}
1118+
let mut otherwise = None;
1119+
for match_pair in match_pairs {
1120+
if let PatKind::Or { ref pats } = *match_pair.pattern.kind {
1121+
let or_span = match_pair.pattern.span;
1122+
let place = &match_pair.place;
1123+
1124+
first_candidate.visit_leaves(|leaf_candidate| {
1125+
self.test_or_pattern(
1126+
leaf_candidate,
1127+
&mut otherwise,
1128+
pats,
1129+
or_span,
1130+
place,
1131+
fake_borrows,
1132+
);
1133+
});
1134+
} else {
1135+
bug!("Or-patterns should have been sorted to the end");
10701136
}
1071-
let remainder_start =
1072-
first_candidate.otherwise_block.unwrap_or_else(|| self.cfg.start_new_block());
1073-
self.match_candidates(
1074-
span,
1075-
remainder_start,
1076-
otherwise_block,
1077-
remaining_candidates,
1078-
fake_borrows,
1079-
)
1080-
} else {
1081-
self.test_candidates(span, candidates, block, otherwise_block, fake_borrows)
10821137
}
1138+
1139+
let remainder_start = otherwise.unwrap_or_else(|| self.cfg.start_new_block());
1140+
1141+
self.match_candidates(
1142+
span,
1143+
remainder_start,
1144+
otherwise_block,
1145+
remaining_candidates,
1146+
fake_borrows,
1147+
)
10831148
}
10841149

10851150
fn test_or_pattern<'pat>(
10861151
&mut self,
10871152
candidate: &mut Candidate<'pat, 'tcx>,
1153+
otherwise: &mut Option<BasicBlock>,
10881154
pats: &'pat [Pat<'tcx>],
10891155
or_span: Span,
10901156
place: &Place<'tcx>,
@@ -1093,27 +1159,18 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
10931159
debug!("test_or_pattern:\ncandidate={:#?}\npats={:#?}", candidate, pats);
10941160
let mut or_candidates: Vec<_> = pats
10951161
.iter()
1096-
.map(|pat| {
1097-
let new_match_pair = smallvec![MatchPair { pattern: pat, place: place.clone() }];
1098-
Candidate {
1099-
span: pat.span,
1100-
has_guard: candidate.has_guard,
1101-
needs_otherwise_block: candidate.needs_otherwise_block,
1102-
match_pairs: new_match_pair,
1103-
bindings: Vec::new(),
1104-
ascriptions: Vec::new(),
1105-
otherwise_block: None,
1106-
pre_binding_block: None,
1107-
next_candidate_pre_binding_block: None,
1108-
subcandidates: Vec::new(),
1109-
}
1110-
})
1162+
.map(|pat| Candidate::new(place.clone(), pat, candidate.has_guard))
11111163
.collect();
11121164
let mut or_candidate_refs: Vec<_> = or_candidates.iter_mut().collect();
1165+
let otherwise = if candidate.otherwise_block.is_some() {
1166+
&mut candidate.otherwise_block
1167+
} else {
1168+
otherwise
1169+
};
11131170
self.match_candidates(
11141171
or_span,
11151172
candidate.pre_binding_block.unwrap(),
1116-
&mut candidate.otherwise_block,
1173+
otherwise,
11171174
&mut or_candidate_refs,
11181175
fake_borrows,
11191176
);
@@ -1128,10 +1185,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
11281185
candidate: &mut Candidate<'_, 'tcx>,
11291186
source_info: SourceInfo,
11301187
) {
1131-
if candidate.subcandidates.is_empty() {
1188+
if candidate.subcandidates.is_empty() || candidate.has_guard {
1189+
// FIXME(or_patterns; matthewjasper) Don't give up if we have a guard.
11321190
return;
11331191
}
1134-
let mut can_merge = !candidate.has_guard;
1192+
1193+
let mut can_merge = true;
11351194

11361195
// Not `Iterator::all` because we don't want to short-circuit.
11371196
for subcandidate in &mut candidate.subcandidates {

‎src/librustc_mir_build/build/matches/simplify.rs

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ use rustc::ty::layout::{Integer, IntegerExt, Size};
2222
use rustc_attr::{SignedInt, UnsignedInt};
2323
use rustc_hir::RangeEnd;
2424

25-
use smallvec::smallvec;
2625
use std::mem;
2726

2827
impl<'a, 'tcx> Builder<'a, 'tcx> {
@@ -49,7 +48,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
4948
if let [MatchPair { pattern: Pat { kind: box PatKind::Or { pats }, .. }, ref place }] =
5049
*match_pairs
5150
{
52-
candidate.subcandidates = self.create_or_subcanidates(candidate, place, pats);
51+
candidate.subcandidates = self.create_or_subcandidates(candidate, place, pats);
5352
return true;
5453
}
5554

@@ -76,26 +75,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
7675
}
7776
}
7877

79-
fn create_or_subcanidates<'pat>(
78+
fn create_or_subcandidates<'pat>(
8079
&mut self,
8180
candidate: &Candidate<'pat, 'tcx>,
8281
place: &Place<'tcx>,
8382
pats: &'pat [Pat<'tcx>],
8483
) -> Vec<Candidate<'pat, 'tcx>> {
8584
pats.iter()
8685
.map(|pat| {
87-
let mut candidate = Candidate {
88-
span: pat.span,
89-
has_guard: candidate.has_guard,
90-
needs_otherwise_block: candidate.needs_otherwise_block,
91-
match_pairs: smallvec![MatchPair { place: place.clone(), pattern: pat }],
92-
bindings: vec![],
93-
ascriptions: vec![],
94-
subcandidates: vec![],
95-
otherwise_block: None,
96-
pre_binding_block: None,
97-
next_candidate_pre_binding_block: None,
98-
};
86+
let mut candidate = Candidate::new(place.clone(), pat, candidate.has_guard);
9987
self.simplify_candidate(&mut candidate);
10088
candidate
10189
})

0 commit comments

Comments
 (0)