-
Notifications
You must be signed in to change notification settings - Fork 496
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce the genericity of many closures #673
Conversation
This fixes the type-length-limit of the specific example in #671, but I'm going to audit for similar cases before we call that fixed. So far, this only takes the max type in that example from ~1.4M characters to ~700K, sneaking under the compiler's default 1M limit, but that's still way too close for comfort. |
1379d12
to
a0dcb4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
annoying that it would be necessary, but I think the approach seems solid
OK, I think we're good now. I swept through pretty much everything, isolating closures wherever I saw there was a type parameter that could be avoided. This is not meant to be an aversion to closures entirely, just where they are implicitly overly generic. |
move |_| f() | ||
} | ||
|
||
join_context(call(oper_a), call(oper_b)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To call out one example -- before, both of those closure wrappers depended on all 4 parameters, <A, B, RA, RB>
, which meant that the join_context
type also carried that duplication. Now the call
wrappers only depend on their respective types, <A, RA>
or <B, RB>
.
I'd be happy to r+ this, but @cuviper did you want to wait until the corresponding PR on rust-lang landed? |
I was ready to wait, but we don't necessarily need to. I was more concerned whether upstream rust thought this approach was reasonable, and since it was approved, that's good enough here. Looks like I have a conflict to resolve here though -- will fix. |
The `take_while` closure only needs to be generic in `T`, along with a captured `&AtomicBool`. When we write the closure directly, it carries all the type baggage of `WhileSomeFolder<'f, C>::consumer_iter<I>`, where the monomorphized type may explode. Instead, we can move this closure to a standalone function for reduced genericity.
Most of these closures only need to be generic in the item type, but when created in the context of some `I` generic iterator, they inherit that genericity. This affects both type length and code duplication.
431c4ba
to
dbc114f
Compare
The rust PR is now merged, so the waiting question is now moot. 😄 bors r=nikomatsakis |
673: Reduce the genericity of many closures r=nikomatsakis a=cuviper The `take_while` closure only needs to be generic in `T`, along with a captured `&AtomicBool`. When we write the closure directly, it carries all the type baggage of `WhileSomeFolder<'f, C>::consumer_iter<I>`, where the monomorphized type may explode. Instead, we can move this closure to a standalone function for reduced genericity. Co-authored-by: Josh Stone <cuviper@gmail.com>
The
take_while
closure only needs to be generic inT
, along with acaptured
&AtomicBool
. When we write the closure directly, it carriesall the type baggage of
WhileSomeFolder<'f, C>::consumer_iter<I>
,where the monomorphized type may explode. Instead, we can move this
closure to a standalone function for reduced genericity.