Skip to content
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

Make function pointers implement traits for up to 12 parameters #28560

Merged
merged 1 commit into from Sep 21, 2015

Conversation

Projects
None yet
9 participants
@Manishearth
Copy link
Member

Manishearth commented Sep 21, 2015

(12 was chosen to be consistent with what we do for tuples)

Fixes #28559

Make function pointers implement traits for up to 12 parameters
(12 was chosen to be consistent with what we do for tuples)

Fixes #28559
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Sep 21, 2015

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@nrc nrc assigned nrc and unassigned aturon Sep 21, 2015

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Sep 21, 2015

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 21, 2015

📌 Commit 5f66c70 has been approved by nrc

bors added a commit that referenced this pull request Sep 21, 2015

Auto merge of #28560 - Manishearth:fix-fnptr-impls, r=nrc
(12 was chosen to be consistent with what we do for tuples)

Fixes #28559
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 21, 2015

⌛️ Testing commit 5f66c70 with merge 6217b00...

@bors bors merged commit 5f66c70 into rust-lang:master Sep 21, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Sep 21, 2015

@Manishearth
Implementations for function pointers were "trimmed" a bit in #28268 to reduce some pretty nasty code bloat. 8 arguments would be enough for backward compatibility with stable.
Well, yes, it required a crater run~~, but who on earth uses functions with 12 arguments?!~~

@Manishearth Manishearth deleted the Manishearth:fix-fnptr-impls branch Sep 21, 2015

@Manishearth

This comment has been minimized.

Copy link
Member Author

Manishearth commented Sep 21, 2015

Functions with a large number of arguments being used in other structs are basically inevitable in bindings.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 21, 2015

It's always good to do a bit of investigation into what caused regressions if they do happen, and as @petrochenkov mentioned we did indeed cut these back to reduce the massive metadata size of libcore. The number chosen for tuples was completely arbitrary, so there's not real reason to match it for functions, and I would prefer to have only the bare minimum necessary to get things working until we find a better solution.

This is ok because it already landed, but always nice to not be too hasty!

@Manishearth

This comment has been minimized.

Copy link
Member Author

Manishearth commented Sep 21, 2015

Will do, sorry about the trouble.

@gkoz

This comment has been minimized.

Copy link
Contributor

gkoz commented Sep 21, 2015

Is there an issue documenting the bloat problem?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Oct 7, 2015

triage: beta-nominated

Nominating for a backport as this fixes a reported regression with chipmunk-sys and is pretty harmless.

@alexcrichton alexcrichton added the T-libs label Oct 7, 2015

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Oct 8, 2015

The libs team decided to accept this for a backport to beta

@brson brson referenced this pull request Oct 16, 2015

Merged

Beta next #29112

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.