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

Emulate variadic methods using tuples #2276

Closed
wants to merge 1 commit into from

Conversation

pvdrz
Copy link
Contributor

@pvdrz pvdrz commented Sep 20, 2022

This PR introduces a new flag --tuple-varargs-len=<LEN> which can be used to emulate variadic methods using tuples of length up to <LEN>.

Example

If the generated function signature for a variadic method is:

extern "C" {
    fn foo_bar(foo: &Foo, a: A, ...);
}

and the --tuple-varargs-len flag is set. A new method for the type Foo is introduced:

impl Foo {
    unsafe fn bar(&self, a: A, varargs: impl VarArgs) {
        varargs.call_foo_bar(self, a)
    }
}

The user would use such method like this:

foo.bar(a, ()); // 0 variadic args
foo.bar(a, (10,)); // 1 variadic arg
foo.bar(a, (10, b"hello")); // 2 variadic args
foo.bar(a, (10, b"hello", false)); // 3 variadic args
...

To do this, the VarArgs trait is declared and implemented automatically up to the value of --tuple-varargs-len:

trait VarArgs {
    unsafe fn call_foo_bar(self, foo: &Foo, a: A);
}

impl VarArgs for () {
    unsafe fn call_foo_bar(self, foo: &Foo, a: A) {
        let () = self;
        foo_bar(foo, a)
    }
}

impl<T0> VarArgs for (T0,) {
    unsafe fn call_foo_bar(self, foo: &Foo, a: A) {
        let (t0, ) = self;
        foo_bar(foo, a, t0)
    }
}

impl<T0, T1> VarArgs for (T0, T1) {
    unsafe fn call_foo_bar(self, foo: &Foo, a: A) {
        let (t0, t1) = self;
        foo_bar(foo, a, t0, t1)
    }
}

This has some disadvantages, such as the user not being able to just do foo.bar(a) or foo,bar(a, v) without getting somewhat confusing errors but this could be mitigated by properly documenting the VarArgs trait.

Fixes #407

@pvdrz pvdrz requested review from amanjeev, emilio and kulp September 20, 2022 20:09
@bors-servo
Copy link

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

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

I'm not sure how I feel about this, do you really think that this is worth it? As in, do you think that doing:

foo.printf(b"%d\0".as_ptr(), (a, b, c))

Is realistically much better than:

Foo_printf(foo, b"%s\0".as_ptr(), a, b, c)

?

@pvdrz
Copy link
Contributor Author

pvdrz commented Sep 23, 2022

The pros I see with approach are:

  1. Method calls are generally shorter: foo.printf( is already shorter than Foo_printf(foo, . Which offsets the cost of adding ( and ) for variadic arguments unless the types and methods have single letter identifiers in which case both are the same length i think.
  2. You don't have to worry about deref: Autoderef makes easier to put Foo behind any kind of smart pointer and still be able to call methods on it. Basically foo.printf( vs Foo_printf(&*foo, .
  3. More consistency while calling methods: In the current state, if you generate bindings for several methods and one of them turns out to be variadic, you have to remember that it will only be available as a free function. The extreme case would be typing foo. and wondering why printf does not appear between your suggestions, then have to go back to the generated bindings and see that Foo_printf is what you need. With this change any variadic method can be called with the dot syntax just like regular methods.

@divagant-martian
Copy link

Oh this is great! As a user this would help in that rust analyzer (or any lsp) will finally suggest these methods

Copy link
Member

@amanjeev amanjeev left a comment

Choose a reason for hiding this comment

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

I approve, but will defer to @emilio.

@pvdrz pvdrz requested a review from emilio September 26, 2022 21:51
@bors-servo
Copy link

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

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

I still tend to think that this is very complex for the value it provides, but at the very least some things need to be fixed.

src/codegen/mod.rs Outdated Show resolved Hide resolved
src/codegen/mod.rs Outdated Show resolved Hide resolved
@pvdrz pvdrz requested a review from emilio October 4, 2022 15:34
@bors-servo
Copy link

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

@bors-servo
Copy link

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

@pvdrz pvdrz closed this Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Find a proper solution for variadic methods when available.
5 participants