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

Preserve argument names #1344

Merged
merged 3 commits into from
Mar 14, 2019
Merged

Preserve argument names #1344

merged 3 commits into from
Mar 14, 2019

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Mar 14, 2019

Part of #1129.

@c410-f3r c410-f3r changed the title [WIP] Preserve argument names Preserve argument names Mar 14, 2019
Copy link
Contributor

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

This is looking pretty good to me, thanks for this! Can you also manually confirm that the names of the arguments for TypeScript are coming through as well?

I think it's reasonable to avoid naming arguments for closures since in general they don't have names, and shim_argument can be safely ignored because it's largely just used for auxiliary internal purposes rather than public-facing-API purposes

crates/cli-support/src/js/js2rust.rs Outdated Show resolved Hide resolved
@@ -106,6 +106,7 @@ macro_rules! shared_api {
}

struct Function<'a> {
arg_names: Vec<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible it'd be great to use &'a str here to allow the encoder/decoder to be pretty lean and avoid the extra String allocations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wish! syn's Ident doesn't expose &str and only implements ToString.

@c410-f3r
Copy link
Contributor Author

Can you also manually confirm that the names of the arguments for TypeScript are coming through as well?

Capture d’écran de 2019-03-14 13-21-32

@alexcrichton
Copy link
Contributor

Ok thanks for checking and clarifying about String, thanks again for the PR!

@alexcrichton alexcrichton merged commit d5a9208 into rustwasm:master Mar 14, 2019
@c410-f3r c410-f3r deleted the arg-names branch March 15, 2019 02:03
@c410-f3r c410-f3r mentioned this pull request Mar 16, 2019
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.

2 participants