Skip to content

Conversation

@gzuidhof
Copy link
Contributor

@gzuidhof gzuidhof commented Jun 8, 2020

From what I can tell, it's 0 for WASM32, not JS.

  • I've read the contributing guidelines

@willemneal
Copy link
Contributor

Perhaps it might be worth setting those values in the enum so that it's more declarative.

@gzuidhof
Copy link
Contributor Author

gzuidhof commented Jun 8, 2020

I agree that's a good idea, I will edit this PR.

@dcodeIO
Copy link
Member

dcodeIO commented Jun 8, 2020

What do you guys think about making the inverse change, i.e. actually make it JS=0, Wasm32=1 and Wasm64? This way, !ASC_TARGET would represent JS (not asc) and ASC_TARGET would be any valid asc target with the option to check which one it is.

@gzuidhof
Copy link
Contributor Author

gzuidhof commented Jun 9, 2020

I think that makes more sense, but it is definitely a breaking change for anybody depending on this value.

@gzuidhof
Copy link
Contributor Author

gzuidhof commented Jun 9, 2020

When not using the portability library, ASC_TARGET is the only value I have to set globally prior to running my program with a JS target. If the JS value is 0 instead I do not have to explicitly set it (I can use !global.ASC_TARGET).

That's why I would prefer JS to be the 0 value.

@dcodeIO
Copy link
Member

dcodeIO commented Jun 9, 2020

Alright, I'm OK with this change as well. From my perspective there'll never be a better time to change the values than now so we break as few projects as possible, which will only become more over time. In particular the largest consumer of the portable lib is the compiler itself, and afaik some benchmarks use it as well but iirc not using ASC_TARGET for it.

Going to merge this PR as it fixes the current behavior, and suggesting to make the switch a new PR that we can pull on the next major release.

@dcodeIO dcodeIO merged commit a27cc54 into AssemblyScript:master Jun 9, 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.

3 participants