Skip to content

Conversation

@ddcc
Copy link
Contributor

@ddcc ddcc commented Jul 23, 2016

This patch is a derivative of #642 that only updates backend data structures and implements an internal API for multiple tables. Parsing/generation of both the binary and s-expression format are unchanged.

@kripken
Copy link
Member

kripken commented Jul 25, 2016

Overall I don't see anything wrong in the code. But what is the goal here? If it's code for experimentation on a side branch that sounds fine, but I don't think this is something we'd want to merge to master unless the wasm spec changed to have multiple tables.

@ddcc
Copy link
Contributor Author

ddcc commented Jul 25, 2016

Correct me if I'm wrong, but I thought that #682 added support for multiple tables, though the MVP only supports one default table per #727?

@kripken
Copy link
Member

kripken commented Jul 25, 2016

I think multiple tables are something to be considered after the MVP, and it's my understanding that all post-MVP features are yet to be figured out. But I could be wrong.

@ddcc
Copy link
Contributor Author

ddcc commented Jul 29, 2016

Is this OK to merge?

@kripken
Copy link
Member

kripken commented Jul 29, 2016

I still have the question from before: while the spec has just one table, I'm not sure we should have multiple table support in binaryen.

It's ok to prototype future features in binaryen. If they add no maintenance burden, the master branch can be fine. But in this case, it's a significant change, so my tendency is to keep it on a side branch.

@ddcc ddcc force-pushed the backend_only branch 4 times, most recently from c3e279a to 1bdda7b Compare August 4, 2016 18:41
@kripken
Copy link
Member

kripken commented Jan 23, 2017

No clear decision to land or not land here. Let's close this for now. Please reopen if things change or if I got it wrong.

@kripken kripken closed this Feb 1, 2017
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