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

Change variant representation? #2546

Closed
chenglou opened this issue Feb 27, 2018 · 2 comments
Closed

Change variant representation? #2546

chenglou opened this issue Feb 27, 2018 · 2 comments

Comments

@chenglou
Copy link
Member

chenglou commented Feb 27, 2018

Currently a variant with multiple constructors carrying payloads are compiled to array with a tag field (an int representing the constructor). This isn't ideal, because serializing the variant* loses that tag field (extra array fields are ignored), and more importantly, various JS tools such as Jest compare the array and neglect the tag (it's rather common to ignore the fact that an array might have extra fields), so e.g. the test assertions are wrong: assert.equal(Foo(1), Bar(1)) passes.

For friendlier interop with existing JS systems, and to keep things simple, I'd say we could:

  1. Change the representation to be [tag, payload1, payload2, ...]. No extra allocation.
  2. Change the representation to be {tag, payload: someTypeHere}. Extra alloc for e.g. constructors with many payload. But more friendly to reading I guess?

* We shouldn't serialize & deserialize the variant and expect things to work, but for temporary iteration this might be fine. Plus, helps a little for e.g. logging where you don't really deserialize.

cc @rickyvetter @cristianoc @jordwalke @bsansouci

@jordwalke
Copy link

Was there some benefit to representing Bar(1) as [1] with a hidden .tag field as opposed to [0, 1]? I can't imagine it actually saves any memory and I'd be worried about the VM doing something unpredictable with a .tag field on an Array. I could be misguided though.

@bobzhang
Copy link
Member

there is a dedicated discussion in #24 . tldr, we chose current representation due to the fact that we don't completely own the compiler pipeline and the compiler internal treat array and block in a same way so that we have to make a trade off somewhere. We may revisit in the future, but not in the short term

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

3 participants