Skip to content

Conversation

@gumb0
Copy link

@gumb0 gumb0 commented Apr 19, 2021

I noticed some tests that were duplicated after #1287.

)

;; Signed LEB128 must not be overlong
(assert_malformed
Copy link
Author

@gumb0 gumb0 Apr 19, 2021

Choose a reason for hiding this comment

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

This one and several tests below are duplicated at:

;; Signed LEB128 must not be overlong

Copy link
Author

Choose a reason for hiding this comment

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

Also

;; Signed LEB128 must not be overlong

)

;; Signed LEB128s sign-extend
(assert_malformed
Copy link
Author

Choose a reason for hiding this comment

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

This one and several tests below are duplicated at:

(assert_malformed

Copy link
Author

Choose a reason for hiding this comment

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

Also

;; Signed LEB128s sign-extend

)

;; Unsigned LEB128s zero-extend
(assert_malformed
Copy link
Author

Choose a reason for hiding this comment

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

Two tests duplicated at

;; Unsigned LEB128s zero-extend

Copy link
Author

Choose a reason for hiding this comment

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

Also

;; Unsigned LEB128s zero-extend

)
"integer representation too long"
)
(assert_malformed
Copy link
Author

Choose a reason for hiding this comment

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

Duplicated at

(assert_malformed

Copy link
Author

Choose a reason for hiding this comment

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

Also

(assert_malformed



;; Unsigned LEB128 must not be overlong
(assert_malformed
Copy link
Author

Choose a reason for hiding this comment

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

Duplicated at

;; Unsigned LEB128 must not be overlong

Copy link
Author

Choose a reason for hiding this comment

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

Also

;; Unsigned LEB128 must not be overlong

"\05\08\01" ;; Memory section with 1 entry
"\00\82\80\80\80\80\00" ;; no max, minimum 2 with one byte too many
)
"integer representation too long"
Copy link
Author

Choose a reason for hiding this comment

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

This test is duplicated at

;; Unsigned LEB128 must not be overlong

In this file I generally deleted the first occurrence and left the second occurrence (because it looked like more tests in the second part). For this test I did the same, but GitHub here shows the diff as if I removed the second occurrence (at line 373)

"type mismatch"
)

(assert_invalid
Copy link
Author

Choose a reason for hiding this comment

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

These are duplicated just above, at lines 389-406.

@gumb0 gumb0 marked this pull request as ready for review April 19, 2021 13:34
rossberg
rossberg previously approved these changes Apr 19, 2021
Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Thanks for spotting! Git merge is dangerous sometimes.

@gumb0
Copy link
Author

gumb0 commented Apr 20, 2021

Now I noticed that many tests added to binary.wast are actually duplications of the old tests in binary-leb128.wast

(assert_malformed (module binary "\00asm" "\01\00\00\00" "\81\00\01\00") "malformed section id")
(assert_malformed (module binary "\00asm" "\01\00\00\00" "\ff\00\01\00") "malformed section id")

;; Unsigned LEB128 can have non-minimal length
Copy link
Author

Choose a reason for hiding this comment

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

;; Unsigned LEB128 can have non-minimal length

"\00\82\80\80\80\00" ;; no max, minimum 2
)

;; Signed LEB128 can have non-minimal length
Copy link
Author

Choose a reason for hiding this comment

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

;; Signed LEB128 can have non-minimal length

"integer representation too long"
)

;; Unsigned LEB128 must not be overlong
Copy link
Author

Choose a reason for hiding this comment

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

;; Unsigned LEB128 must not be overlong

"integer representation too long"
)

;; Signed LEB128 must not be overlong
Copy link
Author

Choose a reason for hiding this comment

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

;; Signed LEB128 must not be overlong

"integer representation too long"
)

;; Unsigned LEB128s zero-extend
Copy link
Author

Choose a reason for hiding this comment

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

;; Unsigned LEB128s zero-extend

)
"integer too large"
)
(assert_malformed
Copy link
Author

Choose a reason for hiding this comment

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

(assert_malformed

"integer too large"
)

;; Signed LEB128s sign-extend
Copy link
Author

Choose a reason for hiding this comment

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

;; Signed LEB128s sign-extend

"integer too large"
)

;; Local number is unsigned 32 bit
Copy link
Author

Choose a reason for hiding this comment

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

This one is duplicated just above.

"\0b" ;; end
)

(module binary
Copy link
Author

Choose a reason for hiding this comment

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

Moved this test and the next one to binary-leb128.wast, because there're other tests checking encoding of table/memory index.

@rossberg
Copy link
Member

rossberg commented Aug 4, 2022

@gumb0, this PR is stale and has conflicts. Do you still intend to land it?

@gumb0 gumb0 force-pushed the remove-duplicate-tests branch from 50cc418 to eea475e Compare August 9, 2022 17:22
@gumb0 gumb0 force-pushed the remove-duplicate-tests branch from eea475e to 5f05d3b Compare August 9, 2022 17:23
@gumb0
Copy link
Author

gumb0 commented Aug 9, 2022

@gumb0, this PR is stale and has conflicts. Do you still intend to land it?

I have rebassed it now and squached two commits, it should be reviewable.

@rossberg
Copy link
Member

Cool, thanks!

@rossberg rossberg merged commit e50d0a7 into WebAssembly:master Aug 10, 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.

2 participants