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

Implement more traits for function pointers #28268

Merged
merged 1 commit into from
Sep 15, 2015
Merged

Conversation

petrochenkov
Copy link
Contributor

Closes #26082

@rust-highfive
Copy link
Collaborator

r? @aturon

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

@alexcrichton
Copy link
Member

Can you measure the metadata size before/after this patch? I'm a little worried about these being so rarely used but having a relatively significant cost in terms of metadata size.

@petrochenkov
Copy link
Contributor Author

Size of libcore.rlib:
before: 20,958,680 bytes
after: 26,644,788 byte
after with all ABIs except for "Rust" and "C" removed: 21,947,498 bytes
after with all ABIs except for "Rust" and "C" and functions with >5 arguments removed: 21,461,438 bytes

@alexcrichton
Copy link
Member

How about we stick to just Rust, C, and <5 arguments for now? I think we can also cut down on the number of traits implemented to the bare bones as well, I don't think these necessarily need to be at the same parity as raw pointers (e.g. are we really using function pointers as keys in hash maps?).

@petrochenkov
Copy link
Contributor Author

How about we stick to just Rust, C, and <5 arguments for now?

Good, it was the back-up plan from the beginning.

are we really using function pointers as keys in hash maps

We aren't, because they don't implement Hash!

I think we can also cut down on the number of traits implemented to the bare bones as well, I don't think these necessarily need to be at the same parity as raw pointer

I'd prefer not to do this, libcore size is not a holy grail after all. What increase in size of libcore do you consider acceptable?
Here's my chart of usefulness for these impls.
Clone (+ 150kb) - was already implemented
PartialEq/Eq (+ 300kb) - was partially implemented
Debug (+ 150kb) - needed if you want to assert_eq that PartialEq above or derive Debug for a containing struct.
PartialOrd/Ord (+ 300kb) or Hash (+ 150kb) are needed to have any kind of set of fn pointers or structs containing them (hash set, tree set, sorted array).
fmt::Pointer (+ 150kb) - needed mostly for consistency with data pointers.

@alexcrichton
Copy link
Member

OK, seems reasonable, I guess we can try to figure out how to control metadata size later.

@bors: r+ bc7c430

Thanks!

@bors
Copy link
Contributor

bors commented Sep 15, 2015

⌛ Testing commit bc7c430 with merge bf60883...

@bors
Copy link
Contributor

bors commented Sep 15, 2015

💔 Test failed - auto-win-gnu-64-opt

@alexcrichton
Copy link
Member

@bors: retry

On Mon, Sep 14, 2015 at 6:38 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-win-gnu-64-opt
http://buildbot.rust-lang.org/builders/auto-win-gnu-64-opt/builds/1409


Reply to this email directly or view it on GitHub
#28268 (comment).

bors added a commit that referenced this pull request Sep 15, 2015
@bors
Copy link
Contributor

bors commented Sep 15, 2015

⌛ Testing commit bc7c430 with merge 8320345...

@bors bors merged commit bc7c430 into rust-lang:master Sep 15, 2015
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 15, 2015
@petrochenkov petrochenkov deleted the fnptr branch September 21, 2015 13:03
bors added a commit that referenced this pull request Jul 19, 2016
Implement traits for variadic function pointers

Closes #34874
cc #28268

r? @alexcrichton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants