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

Validate functions' arity #433

Merged
merged 3 commits into from
Jul 22, 2020
Merged

Validate functions' arity #433

merged 3 commits into from
Jul 22, 2020

Conversation

gumb0
Copy link
Collaborator

@gumb0 gumb0 commented Jul 21, 2020

No description provided.

@gumb0 gumb0 marked this pull request as ready for review July 21, 2020 17:19
@@ -13,6 +13,15 @@
using namespace fizzy;
using namespace fizzy::test;

TEST(validation, imported_function_type_invalid_arity)
{
/* wat2wasm
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wabt doesn't throw an error unless function with this type is defined

Copy link
Member

Choose a reason for hiding this comment

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

Can you add this to our list of "to reach out"? Or just ask the question on the wabt repo? It may be we are using an old wabt and it was fixed since, or perhaps they are more committed to the multi-return-value change (I believe that is implemented by wabt).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried it with recent wabt 1.0.19, and it appears to be fixed: multi-value is enabled by default there, but with --disable-multi-value this correctly throws an error.

I think I'll add --no-check here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

1.0.14 is the last one that has multi-value disabled by default, and it has this issue fixed also.

@gumb0 gumb0 requested review from chfast and axic July 21, 2020 17:30
@codecov
Copy link

codecov bot commented Jul 21, 2020

Codecov Report

Merging #433 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #433   +/-   ##
=======================================
  Coverage   99.32%   99.32%           
=======================================
  Files          50       50           
  Lines       13978    13989   +11     
=======================================
+ Hits        13883    13894   +11     
  Misses         95       95           

@@ -13,6 +13,15 @@
using namespace fizzy;
using namespace fizzy::test;

TEST(validation, imported_function_type_invalid_arity)
Copy link
Member

Choose a reason for hiding this comment

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

This is not imported, just function_type_invalid_arity

@gumb0 gumb0 merged commit 62aaee7 into master Jul 22, 2020
@gumb0 gumb0 deleted the validate-func-arity branch July 22, 2020 10:14
@gumb0 gumb0 mentioned this pull request Jul 22, 2020
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