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

Separation into classes? #194

Closed
kripken opened this issue Nov 24, 2015 · 4 comments
Closed

Separation into classes? #194

kripken opened this issue Nov 24, 2015 · 4 comments

Comments

@kripken
Copy link
Member

kripken commented Nov 24, 2015

I am having a hard time deciding how to represent WebAssembly in a C++ class hierarchy in binaryen, and would appreciate advice. Right now I have a separate class for call and call_import, for example, although I have just one if whose else might be null, instead of two classes, and that feels inconsistent, but I'm not sure which way to refactor it.

I also see that we have br and br_if which have an optional value argument. That seems to be more consistent with having one if class with an optional argument. Is that intended?

@sunfishcode
Copy link
Member

On call vs. call_import, there is an open design issue.

@titzer
Copy link
Contributor

titzer commented Nov 25, 2015

FWIW in v8-native, we avoid having a heavyweight class-per-structure and
have a list of all opcodes and their input/output types (i.e. signature).
That is mostly because v8-native doesn't ever really reify the AST, but
decodes bytes directly either to validate or to generate an IR graph.

On Wed, Nov 25, 2015 at 12:59 AM, Dan Gohman notifications@github.com
wrote:

On call vs. call_import, there is an open design issue
WebAssembly/design#421.


Reply to this email directly or view it on GitHub
#194 (comment).

@rossberg
Copy link
Member

FWIW, the nature of the else operand to if is quite different from the ones to br and friends. Conceptually, the else-less if is just a syntactic shorthand. Still, it makes sense to unify the conditionals in an internal AST (that's what the spec does as well).

The arguments to br, return et al are different, though. They stand for 0 or 1 value being passed. In a potential post-MVP extension with multiple return values, these would naturally generalise to n-ary lists. From that perspective, it does not make sense to treat different arities as separate operators, internally or externally.

In general, I'd argue that the factorisation of operators as described by AstSemantics is oriented more towards reflecting the concrete binary format and certain decoding considerations than what you'd likely represent with an actual AST. So it would seem natural to structure things somewhat differently in a real-world AST-based implementation.

@kripken
Copy link
Member Author

kripken commented Nov 26, 2015

Thanks for the feedback, everyone. Ok, it sounds like there isn't really a "right" way to do this from the spec's perspective, and specific implementations might make different decisions. Not sure yet what I'll do in binaryen, but I lean towards optimizing the AST for code size.

@kripken kripken closed this as completed Nov 26, 2015
ngzhian pushed a commit to ngzhian/spec that referenced this issue Nov 4, 2021
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

No branches or pull requests

4 participants