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

Fix trailing undefined for optional parameters not omitted with @send #6716

Merged

Conversation

cknitt
Copy link
Member

@cknitt cknitt commented Apr 6, 2024

Fixes #6715

@cknitt cknitt changed the title Fix trailing undefined send Fix trailing undefined for optional parameters not omitted with @send Apr 6, 2024
@cknitt cknitt requested review from zth and cristianoc April 6, 2024 06:53
Copy link
Collaborator

@zth zth left a comment

Choose a reason for hiding this comment

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

Nice find! I wonder if there are more code paths where it should be applied...

@cknitt
Copy link
Member Author

cknitt commented Apr 6, 2024

@zth I now moved the handling directly to assemble_args_no_splice so it applies to more cases, e.g., @new. @get/@set/@get_index/@set_index also use this function, but I think it doesn't really matter there.

Not sure what the cases Js_module_as_fn and Js_module_as_class are.

@cknitt cknitt requested a review from zth April 6, 2024 13:09
@cknitt
Copy link
Member Author

cknitt commented Apr 6, 2024

Also, I was wondering: External function calls with trailing undefineds will probably be the exception rather than the rule, but we are now doing additional list allocations whenever we emit code for any external function call - should we maybe first check if there are any trailing undefineds before doing these list computations?

@zth
Copy link
Collaborator

zth commented Apr 6, 2024

I'll let @cristianoc have a final look at this too, looks good from my side.

@cknitt
Copy link
Member Author

cknitt commented Apr 6, 2024

Added check has_undefined_trailing_args before during the List.rev stuff.

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.

None yet

3 participants