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

Add new compound Rtt type #3076

Merged
merged 10 commits into from
Aug 26, 2020
Merged

Add new compound Rtt type #3076

merged 10 commits into from
Aug 26, 2020

Conversation

dcodeIO
Copy link
Contributor

@dcodeIO dcodeIO commented Aug 24, 2020

Extends compound types introduced in #3012 with a representation of Rtts as described in the GC proposal, by also introducing the concept of a HeapType shared between TypeInfo and Rtt. Again, this should be a non-functional change since Rtts are not used anywhere yet. Subtyping rules and updating the xref aliases is left for future work.

src/wasm-type.h Outdated Show resolved Hide resolved
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

This is looking great so far 👍

src/wasm/wasm-type.cpp Show resolved Hide resolved
src/wasm-type.h Show resolved Hide resolved
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Very nice! It looks like separating out HeapType was a good move 👍

src/wasm/wasm-type.cpp Show resolved Hide resolved
@tlively
Copy link
Member

tlively commented Aug 25, 2020

Let me know if you think this is ready for final review. Leaving subtyping to a follow-on PR would be fine, if you'd like.

@dcodeIO
Copy link
Contributor Author

dcodeIO commented Aug 26, 2020

With the remaining tests added this should be ready for review then :)

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Looks good to me besides that one last comment. Feel free to land it when it's ready; you should have write access now.

return array;
}
std::string toString() const;
};
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to move the less trivial methods into the .cpp file. I'm thinking roughly anything with a switch in it. My goal there would be consistency with where the TypeInfo methods are placed.

@dcodeIO dcodeIO merged commit a61b9df into WebAssembly:master Aug 26, 2020
@dcodeIO
Copy link
Contributor Author

dcodeIO commented Aug 26, 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