Skip to content

Do not return a no-op function if default function isn't found#1927

Closed
AlexEne wants to merge 1 commit into
bytecodealliance:mainfrom
AlexEne:main
Closed

Do not return a no-op function if default function isn't found#1927
AlexEne wants to merge 1 commit into
bytecodealliance:mainfrom
AlexEne:main

Conversation

@AlexEne
Copy link
Copy Markdown
Contributor

@AlexEne AlexEne commented Jun 26, 2020

This PR changes the linker.get_default() deafult return value. It removes the return of a no-op function as this is rarely what users of the method would want. I have replaced it with an error that indicates that the module has no default functions to return.

This has been briefly discussed in the zulip bytecode alliance channel.

I do not know how to add test cases to this, it seems that there were no tests around it, but I'm happy to add some if anyone could guide me on where to put them.

I am unsure who the right reviewer for this is.

@AlexEne AlexEne changed the title Do not return a no-op if default function isn't found Do not return a no-op function if default function isn't found Jun 26, 2020
@AlexEne
Copy link
Copy Markdown
Contributor Author

AlexEne commented Jun 26, 2020

It seems I have missed some tests, right now the following are failing:

    cli_tests::greeter_preload_command
    cli_tests::minimal_reactor

@github-actions github-actions Bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jun 26, 2020
@github-actions
Copy link
Copy Markdown

Subscribe to Label Action

cc @peterhuene

Details This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@abrown
Copy link
Copy Markdown
Member

abrown commented Jun 26, 2020

Maybe @alexcrichton?

@alexcrichton alexcrichton requested a review from sunfishcode June 26, 2020 16:32
@alexcrichton
Copy link
Copy Markdown
Member

I've requested review from @sunfishcode for this

@AlexEne
Copy link
Copy Markdown
Contributor Author

AlexEne commented Jun 29, 2020

@sunfishcode if you're ok with this change I'll go ahead and fix the tests.

@sunfishcode
Copy link
Copy Markdown
Member

This change looks good to me. This is a space that we're still figuring out as we go; I think making it an error now makes sense.

@AlexEne
Copy link
Copy Markdown
Contributor Author

AlexEne commented Aug 10, 2020

Ok, I'll fix the tests and update this PR in the next days

@bjorn3
Copy link
Copy Markdown
Contributor

bjorn3 commented Jun 12, 2021

This will need to be updated for #2897. @AlexEne do you want to do this or should I take it over?

@AlexEne
Copy link
Copy Markdown
Contributor Author

AlexEne commented Jun 15, 2021

Oh wow, I forgot about this. So sorry. I would be able to pick it up again later next week, but I'm swamped with other stuff so if you want to land this it may be faster to do it.

It's not a massive change and waiting for me isn't really necessary.

@bjorn3
Copy link
Copy Markdown
Contributor

bjorn3 commented Jun 15, 2021

I don't have any immediate need for this. I was just triaging the PR queue a bit.

@AlexEne
Copy link
Copy Markdown
Contributor Author

AlexEne commented Jun 15, 2021

Ok, I've set up some time to update this PR on Friday next week.

@AlexEne
Copy link
Copy Markdown
Contributor Author

AlexEne commented Jun 25, 2021

Closing this and will re-open a correct one on the latest version

@AlexEne AlexEne closed this Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wasmtime:api Related to the API of the `wasmtime` crate itself

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants