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.

Sign up for GitHub

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

Jump to bottom

rustc_ast_passes: allow c-variadic associated fns #74765

Closed
wants to merge 1 commit into from
Closed

rustc_ast_passes: allow c-variadic associated fns #74765

wants to merge 1 commit into from

Conversation

dlrobertson
Copy link
Contributor

@dlrobertson dlrobertson commented Jul 26, 2020 •
edited
Loading

  • Allow c-variadic associated functions
  • Add test for c-variadic functions in the Impl context

Related To: #44930
CC: @jethrogb @sarvi
r? @joshtriplett

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 26, 2020
@dlrobertson dlrobertson force-pushed the c-variadic-assoc-fn branch from 01bb58e to 7073b9d Compare July 26, 2020 00:54
joshtriplett
joshtriplett requested changes Jul 26, 2020
View reviewed changes
Copy link
Member

@joshtriplett joshtriplett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add some tests that the functions work as expected, retrieving passed variadic arguments.

Ideally, I would love to see the same tests away used for free functions applied to these cases too, perhaps with a macro to avoid duplication.

@nikomatsakis nikomatsakis mentioned this pull request Jul 27, 2020
Open
3 tasks
@dlrobertson
Copy link
Contributor Author

Please also add some tests that the functions work as expected, retrieving passed variadic arguments.

Ideally, I would love to see the same tests away used for free functions applied to these cases too, perhaps with a macro to avoid duplication.

Sounds good.

@dlrobertson dlrobertson force-pushed the c-variadic-assoc-fn branch from 7073b9d to 3cb6abb Compare August 1, 2020 22:25
@dlrobertson
Copy link
Contributor Author

@joshtriplett added a run-pass test. Is this what you're looking for?

@bors
Copy link
Collaborator

bors commented Aug 30, 2020

☔ The latest upstream changes (presumably #74862) made this pull request unmergeable. Please resolve the merge conflicts.

@crlf0710
Copy link
Member

@dlrobertson Ping from triage, could you address the review comments? thanks

@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 18, 2020
@dlrobertson
rustc_ast_passes: allow c-variadic associated fns
96a8048 
 - Allow c-variadic associated functions
 - Add test for c-variadic functions in the Impl context
@dlrobertson dlrobertson force-pushed the c-variadic-assoc-fn branch 2 times, most recently from 8b0d291 to 96a8048 Compare September 18, 2020 13:29
@dlrobertson
Copy link
Contributor Author

@crlf0710 thanks for the ping, I've also rebased on master.

@joshtriplett I added a run-pass test, but I didn't change the run-make tests. After looking into this a bit more I realized that if the output is a c library we're not going to make symbol foo visible for the function Foo::foo. So I think the only case that this is valid for is a case where the variadics are going to be used entirely within rust.

@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 3, 2020
@Dylan-DPC-zz
Copy link

@joshtriplett this is ready for review

@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 3, 2020

📌 Commit 96a8048 has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2020
@jonas-schievink jonas-schievink mentioned this pull request Oct 3, 2020
Closed
@jonas-schievink
Copy link
Contributor

@bors r- failed in #77509 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 3, 2020
@dlrobertson
Copy link
Contributor Author

Looking at this now... not immediately sure why this would fail on arm-android

@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. A-associated-items Area: Associated items (types, constants & functions) A-FFI Area: Foreign function interface (FFI) F-c_variadic `#![feature(c_variadic)]` and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 30, 2020
@Dylan-DPC-zz
Copy link

@dlrobertson any updates?

@crlf0710
Copy link
Member

crlf0710 commented Dec 4, 2020

@dlrobertson Ping from triage: What's the current status of this?

@dlrobertson
Copy link
Contributor Author

No updates on why this is failing on android, but after looking at this more I'm not sure why we would add this. A member function wouldn't be exported right? So really this would only be used for a Rust C-variadic function that was used in another rust function. Would that be considered an anti-pattern?

CC @joshtriplett

@tmiasko
Copy link
Contributor

tmiasko commented Dec 5, 2020 •
edited
Loading

The CStr::from_ptr takes *const c_char as an argument. Whether the char type signed or unsigned is implementation defined in C, and on Android happens to be different from what is being assumed in the test case here.

@joshtriplett
Copy link
Member

@dlrobertson wrote:

No updates on why this is failing on android, but after looking at this more I'm not sure why we would add this. A member function wouldn't be exported right? So really this would only be used for a Rust C-variadic function that was used in another rust function. Would that be considered an anti-pattern?

This isn't about member functions; this is about associated functions (which don't take self). There are good reasons to want to have a C-compatible associated function; you might pass such a function as a pointer, for instance.

@JohnCSimon
Copy link
Member

Ping from triage
@dlrobertson - hi - is this pull request ready for review? if so can you alert the reviewer, if not, please address the comments. thanks!

@crlf0710
Copy link
Member

@dlrobertson Triage: I'm closing this due to inactivity. Feel free to reopen or create a new pr when you've got time to work on this again. Thanks!

@crlf0710 crlf0710 closed this Jan 29, 2021
@crlf0710 crlf0710 added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshtriplett joshtriplett joshtriplett approved these changes

Assignees

@joshtriplett joshtriplett

Labels
A-associated-items Area: Associated items (types, constants & functions) A-FFI Area: Foreign function interface (FFI) F-c_variadic `#![feature(c_variadic)]` S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@dlrobertson @bors @crlf0710 @Dylan-DPC-zz @joshtriplett @jonas-schievink @tmiasko @JohnCSimon @rust-highfive @camelid