Skip to content

Commit aeab250

Browse files
committed
auto merge of #15503 : pnkfelix/rust/fsk-linear-deriving-partialord, r=huonw
Instead of generating a separate case (albeit trivial) for each of the N*N cases when comparing two instances of an enum with N variants, this `deriving` uses the strategy outlined here: #15375 (comment) In particular, it generates code that looks more like this: ```rust match (this, that, ...) { (Variant1, Variant1, Variant1) => ... // delegate Matching on Variant1 (Variant2, Variant2, Variant2) => ... // delegate Matching on Variant2 ... _ => { let index_tup = { let idx_this = match this { Variant1 => 0u, Variant2 => 1u, ... }; let idx_that = match that { Variant1 => 0u, Variant2 => 1u, ... }; ... (idx_this, idx_that, ...) }; ... // delegate to catch-all; it can inspect `index_tup` for its logic } } ``` While adding a new variant to the `const_nonmatching` flag (and renaming it to `on_nonmatching`) to allow expressing the above (while still allowing one to opt back into the old `O(N^2)` and in general `O(N^K)` (where `K` is the number of self arguments) code generation behavior), I had two realizations: 1. Observation: Nothing except for the comparison derivings (`PartialOrd`, `Ord`, `PartialEq`, `Eq`) were even using the old `O(N^K)` code generator. So after this hypothetically lands, *nothing* needs to use them, and thus that code generation strategy could be removed, under the assumption that it is very unlikely that any `deriving` mode will actually need that level of generality. 2. Observation: The new code generator I am adding can actually be unified with all of the other code generators that just dispatch on the variant tag (they all assume that there is only one self argument). These two observations mean that one can get rid of the `const_nonmatching` (aka `on_nonmatching`) entirely. So I did that too in this PR. The question is: Do we actually want to follow through on both of the above observations? I'm pretty sure the second observation is a pure win. But there *might* be someone out there with an example that invalidates the reasoning in the first observation. That is, there might be a client out there with an example of hypothetical deriving mode that wants to opt into the `O(N^K)` behavior. So, if that is true, then I can revise this PR to resurrect the `on_nonmatching` flag and provide a way to access the `O(N^K)` behavior. The manner in which I choose to squash these commits during a post-review rebase depends on the answer to the above question. Fix #15375.
2 parents b8059f7 + 5cee578 commit aeab250

File tree

14 files changed

+399
-293
lines changed

14 files changed

+399
-293
lines changed

src/libsyntax/ext/deriving/clone.rs

+1-2
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ pub fn expand_deriving_clone(cx: &mut ExtCtxt,
3939
args: Vec::new(),
4040
ret_ty: Self,
4141
attributes: attrs,
42-
const_nonmatching: false,
4342
combine_substructure: combine_substructure(|c, s, sub| {
4443
cs_clone("Clone", c, s, sub)
4544
}),
@@ -69,7 +68,7 @@ fn cs_clone(
6968
ctor_ident = variant.node.name;
7069
all_fields = af;
7170
},
72-
EnumNonMatching(..) => {
71+
EnumNonMatchingCollapsed (..) => {
7372
cx.span_bug(trait_span,
7473
format!("non-matching enum variants in \
7574
`deriving({})`",

src/libsyntax/ext/deriving/cmp/eq.rs

-1
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,6 @@ pub fn expand_deriving_eq(cx: &mut ExtCtxt,
4545
args: vec!(borrowed_self()),
4646
ret_ty: Literal(Path::new(vec!("bool"))),
4747
attributes: attrs,
48-
const_nonmatching: true,
4948
combine_substructure: combine_substructure(|a, b, c| {
5049
$f(a, b, c)
5150
})

src/libsyntax/ext/deriving/cmp/ord.rs

+35-34
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ pub fn expand_deriving_ord(cx: &mut ExtCtxt,
3535
args: vec!(borrowed_self()),
3636
ret_ty: Literal(Path::new(vec!("bool"))),
3737
attributes: attrs,
38-
const_nonmatching: false,
3938
combine_substructure: combine_substructure(|cx, span, substr| {
4039
cs_op($op, $equal, cx, span, substr)
4140
})
@@ -59,7 +58,6 @@ pub fn expand_deriving_ord(cx: &mut ExtCtxt,
5958
args: vec![borrowed_self()],
6059
ret_ty: ret_ty,
6160
attributes: attrs,
62-
const_nonmatching: false,
6361
combine_substructure: combine_substructure(|cx, span, substr| {
6462
cs_partial_cmp(cx, span, substr)
6563
})
@@ -82,24 +80,33 @@ pub fn expand_deriving_ord(cx: &mut ExtCtxt,
8280
trait_def.expand(cx, mitem, item, push)
8381
}
8482

85-
pub fn some_ordering_const(cx: &mut ExtCtxt, span: Span, cnst: Ordering) -> Gc<ast::Expr> {
86-
let cnst = match cnst {
87-
Less => "Less",
88-
Equal => "Equal",
89-
Greater => "Greater"
83+
pub enum OrderingOp {
84+
PartialCmpOp, LtOp, LeOp, GtOp, GeOp,
85+
}
86+
87+
pub fn some_ordering_collapsed(cx: &mut ExtCtxt,
88+
span: Span,
89+
op: OrderingOp,
90+
self_arg_tags: &[ast::Ident]) -> Gc<ast::Expr> {
91+
let lft = cx.expr_ident(span, self_arg_tags[0]);
92+
let rgt = cx.expr_addr_of(span, cx.expr_ident(span, self_arg_tags[1]));
93+
let op_str = match op {
94+
PartialCmpOp => "partial_cmp",
95+
LtOp => "lt", LeOp => "le",
96+
GtOp => "gt", GeOp => "ge",
9097
};
91-
let ordering = cx.path_global(span,
92-
vec!(cx.ident_of("std"),
93-
cx.ident_of("cmp"),
94-
cx.ident_of(cnst)));
95-
let ordering = cx.expr_path(ordering);
96-
cx.expr_some(span, ordering)
98+
cx.expr_method_call(span, lft, cx.ident_of(op_str), vec![rgt])
9799
}
98100

99101
pub fn cs_partial_cmp(cx: &mut ExtCtxt, span: Span,
100102
substr: &Substructure) -> Gc<Expr> {
101103
let test_id = cx.ident_of("__test");
102-
let equals_expr = some_ordering_const(cx, span, Equal);
104+
let ordering = cx.path_global(span,
105+
vec!(cx.ident_of("std"),
106+
cx.ident_of("cmp"),
107+
cx.ident_of("Equal")));
108+
let ordering = cx.expr_path(ordering);
109+
let equals_expr = cx.expr_some(span, ordering);
103110

104111
/*
105112
Builds:
@@ -141,13 +148,11 @@ pub fn cs_partial_cmp(cx: &mut ExtCtxt, span: Span,
141148
cx.expr_block(cx.block(span, vec!(assign), Some(if_)))
142149
},
143150
equals_expr.clone(),
144-
|cx, span, list, _| {
145-
match list {
146-
// an earlier nonmatching variant is Less than a
147-
// later one.
148-
[(self_var, _, _), (other_var, _, _)] =>
149-
some_ordering_const(cx, span, self_var.cmp(&other_var)),
150-
_ => cx.span_bug(span, "not exactly 2 arguments in `deriving(Ord)`")
151+
|cx, span, (self_args, tag_tuple), _non_self_args| {
152+
if self_args.len() != 2 {
153+
cx.span_bug(span, "not exactly 2 arguments in `deriving(Ord)`")
154+
} else {
155+
some_ordering_collapsed(cx, span, PartialCmpOp, tag_tuple)
151156
}
152157
},
153158
cx, span, substr)
@@ -191,19 +196,15 @@ fn cs_op(less: bool, equal: bool, cx: &mut ExtCtxt, span: Span,
191196
cx.expr_binary(span, ast::BiOr, cmp, and)
192197
},
193198
cx.expr_bool(span, equal),
194-
|cx, span, args, _| {
195-
// nonmatching enums, order by the order the variants are
196-
// written
197-
match args {
198-
[(self_var, _, _),
199-
(other_var, _, _)] =>
200-
cx.expr_bool(span,
201-
if less {
202-
self_var < other_var
203-
} else {
204-
self_var > other_var
205-
}),
206-
_ => cx.span_bug(span, "not exactly 2 arguments in `deriving(Ord)`")
199+
|cx, span, (self_args, tag_tuple), _non_self_args| {
200+
if self_args.len() != 2 {
201+
cx.span_bug(span, "not exactly 2 arguments in `deriving(Ord)`")
202+
} else {
203+
let op = match (less, equal) {
204+
(true, true) => LeOp, (true, false) => LtOp,
205+
(false, true) => GeOp, (false, false) => GtOp,
206+
};
207+
some_ordering_collapsed(cx, span, op, tag_tuple)
207208
}
208209
},
209210
cx, span, substr)

src/libsyntax/ext/deriving/cmp/totaleq.rs

-1
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@ pub fn expand_deriving_totaleq(cx: &mut ExtCtxt,
5757
args: vec!(),
5858
ret_ty: nil_ty(),
5959
attributes: attrs,
60-
const_nonmatching: true,
6160
combine_substructure: combine_substructure(|a, b, c| {
6261
cs_total_eq_assert(a, b, c)
6362
})

src/libsyntax/ext/deriving/cmp/totalord.rs

+15-23
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ use ext::deriving::generic::*;
1717
use ext::deriving::generic::ty::*;
1818
use parse::token::InternedString;
1919

20-
use std::cmp::{Ordering, Equal, Less, Greater};
2120
use std::gc::Gc;
2221

2322
pub fn expand_deriving_totalord(cx: &mut ExtCtxt,
@@ -41,7 +40,6 @@ pub fn expand_deriving_totalord(cx: &mut ExtCtxt,
4140
args: vec!(borrowed_self()),
4241
ret_ty: Literal(Path::new(vec!("std", "cmp", "Ordering"))),
4342
attributes: attrs,
44-
const_nonmatching: false,
4543
combine_substructure: combine_substructure(|a, b, c| {
4644
cs_cmp(a, b, c)
4745
}),
@@ -53,22 +51,21 @@ pub fn expand_deriving_totalord(cx: &mut ExtCtxt,
5351
}
5452

5553

56-
pub fn ordering_const(cx: &mut ExtCtxt, span: Span, cnst: Ordering) -> ast::Path {
57-
let cnst = match cnst {
58-
Less => "Less",
59-
Equal => "Equal",
60-
Greater => "Greater"
61-
};
62-
cx.path_global(span,
63-
vec!(cx.ident_of("std"),
64-
cx.ident_of("cmp"),
65-
cx.ident_of(cnst)))
54+
pub fn ordering_collapsed(cx: &mut ExtCtxt,
55+
span: Span,
56+
self_arg_tags: &[ast::Ident]) -> Gc<ast::Expr> {
57+
let lft = cx.expr_ident(span, self_arg_tags[0]);
58+
let rgt = cx.expr_addr_of(span, cx.expr_ident(span, self_arg_tags[1]));
59+
cx.expr_method_call(span, lft, cx.ident_of("cmp"), vec![rgt])
6660
}
6761

6862
pub fn cs_cmp(cx: &mut ExtCtxt, span: Span,
6963
substr: &Substructure) -> Gc<Expr> {
7064
let test_id = cx.ident_of("__test");
71-
let equals_path = ordering_const(cx, span, Equal);
65+
let equals_path = cx.path_global(span,
66+
vec!(cx.ident_of("std"),
67+
cx.ident_of("cmp"),
68+
cx.ident_of("Equal")));
7269

7370
/*
7471
Builds:
@@ -110,16 +107,11 @@ pub fn cs_cmp(cx: &mut ExtCtxt, span: Span,
110107
cx.expr_block(cx.block(span, vec!(assign), Some(if_)))
111108
},
112109
cx.expr_path(equals_path.clone()),
113-
|cx, span, list, _| {
114-
match list {
115-
// an earlier nonmatching variant is Less than a
116-
// later one.
117-
[(self_var, _, _),
118-
(other_var, _, _)] => {
119-
let order = ordering_const(cx, span, self_var.cmp(&other_var));
120-
cx.expr_path(order)
121-
}
122-
_ => cx.span_bug(span, "not exactly 2 arguments in `deriving(Ord)`")
110+
|cx, span, (self_args, tag_tuple), _non_self_args| {
111+
if self_args.len() != 2 {
112+
cx.span_bug(span, "not exactly 2 arguments in `deriving(TotalOrd)`")
113+
} else {
114+
ordering_collapsed(cx, span, tag_tuple)
123115
}
124116
},
125117
cx, span, substr)

src/libsyntax/ext/deriving/decodable.rs

-1
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ pub fn expand_deriving_decodable(cx: &mut ExtCtxt,
5454
vec!(box Self,
5555
box Literal(Path::new_local("__E"))), true)),
5656
attributes: Vec::new(),
57-
const_nonmatching: true,
5857
combine_substructure: combine_substructure(|a, b, c| {
5958
decodable_substructure(a, b, c)
6059
}),

src/libsyntax/ext/deriving/default.rs

-1
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ pub fn expand_deriving_default(cx: &mut ExtCtxt,
3939
args: Vec::new(),
4040
ret_ty: Self,
4141
attributes: attrs,
42-
const_nonmatching: false,
4342
combine_substructure: combine_substructure(|a, b, c| {
4443
default_substructure(a, b, c)
4544
})

src/libsyntax/ext/deriving/encodable.rs

-1
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,6 @@ pub fn expand_deriving_encodable(cx: &mut ExtCtxt,
121121
box Literal(Path::new_local("__E"))),
122122
true)),
123123
attributes: Vec::new(),
124-
const_nonmatching: true,
125124
combine_substructure: combine_substructure(|a, b, c| {
126125
encodable_substructure(a, b, c)
127126
}),

0 commit comments

Comments
 (0)