-
Notifications
You must be signed in to change notification settings - Fork 745
codegen: Don't implement variadic methods. #403
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
Conversation
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.
Something is funky here. The changes look right to me, but the test code looks wrong.
// Do not generate variadic methods, since rust does not allow | ||
// implementing them, and we don't do a good job at it anyway. | ||
if signature.is_variadic() { | ||
return; |
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.
Might be worth a debug!(...)
here.
} | ||
extern "C" { | ||
#[link_name = "_ZN3Bar3fooEPKcz"] | ||
pub fn Bar_foo(this: *mut Bar, fmt: *const ::std::os::raw::c_char, ...); |
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.
I think the point was supposed to be not to generate this, right?
First of all, we can't support variadics. Second of all, is this codegen'd a named type named ...
? Need to implement the checks that named types are actually valid named types...
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.
Nope, the point was to not generate the wrapper method that calls this. Generating variadic functions otherwise is fine.
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.
(And nope, it's not a named type, rust's AST can represent variadic functions, which is what we use here)
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.
Hm yeah I guess the tests are passing which means it compiles. Color me surprised.
@bors-servo r+ |
📌 Commit b743ab0 has been approved by |
☀️ Test successful - status-travis |
Fixes #402
r? @fitzgen