Skip to content

Conversation

@zherczeg
Copy link
Contributor

@zherczeg zherczeg commented Apr 3, 2025

Code is based on the work of SoniEx2.

Result result = PeekType(0, &type);
if (!type.IsReferenceWithIndex()) {
type = Type::Reference;
type = Type(Type::Reference, kInvalidIndex);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was inconsistent in the code, sometimes Type(Type::Reference, kInvalidIndex) is used, sometimes just a Type::Reference. Now we must use the Type(Type::Reference, kInvalidIndex) form.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sounds good!

}
constexpr operator Enum() const { return enum_; }

friend constexpr bool operator==(const Type a, const Type b) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is Type or Type& is better?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Type is probably better, it's a pair of 32-bit values after all. Any modern compiler should be able to optimize it.

Code is based on the work of SoniEx2.
@zherczeg
Copy link
Contributor Author

zherczeg commented Apr 3, 2025

Thank you. Do you need more changes?

Copy link
Collaborator

@SoniEx2 SoniEx2 left a comment

Choose a reason for hiding this comment

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

Good thought to make Type(Type::Any) match zero-initialization. We wonder where that's being relied on (an union not being explicitly set?), but otherwise, well done!

@SoniEx2 SoniEx2 merged commit d30f043 into WebAssembly:main Apr 3, 2025
18 checks passed
@zherczeg zherczeg deleted the type_compare branch April 3, 2025 14:41
@zherczeg
Copy link
Contributor Author

zherczeg commented Apr 3, 2025

The T{} template constructor does it.
https://github.com/WebAssembly/wabt/blob/main/src/shared-validator.cc#L342

@zherczeg
Copy link
Contributor Author

zherczeg commented Apr 3, 2025

Thank you!

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