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

fix #16771 (swap js); improve tsystem_misc.nim; test in js too #16778

Closed

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Jan 21, 2021

future work

(EDIT) links

@timotheecour timotheecour changed the title fix #16771; improve tsystem_misc.nim; test in js too fix #16771 (swap js); improve tsystem_misc.nim; test in js too Jan 21, 2021
@timotheecour timotheecour requested a review from Clyybber January 21, 2021 03:40
@Clyybber
Copy link
Contributor

This is a hack/workaround, instead genSwap should be fixed here: https://github.com/nim-lang/Nim/blob/devel/compiler/jsgen.nim#L1119

@timotheecour
Copy link
Member Author

timotheecour commented Jan 21, 2021

This is a hack/workaround,

how so? What are benefits of jsgen.genSwap? jsgen.genSwap is more complex and it's better to use regular code than rely on magics, all else being equal. If this PR is accepted, we can remove jsgen.genSwap in a followup PR.

@Clyybber
Copy link
Contributor

Clyybber commented Jan 21, 2021

Then please use a temporary instead of this approach; related: https://web.archive.org/web/20151229220921/http://jsperf.com/swap-array-vs-variable/34

@Araq
Copy link
Member

Araq commented Jan 21, 2021

how so? What are benefits of jsgen.genSwap?

Instead of adding more hacks to poor system.nim we should fix the JS codegen. system.nim bloat is a real problem and causes on-going costs.

@ringabout

This comment was marked as outdated.

@Araq
Copy link
Member

Araq commented Feb 1, 2021

Well without a corresponding bugfix for vmgen, nobody knows how many hacks it involves.

@timotheecour
Copy link
Member Author

timotheecour commented Feb 2, 2021

Well without a corresponding bugfix for vmgen, nobody knows how many hacks it involves.

the remaining vm issue (2nd half of #16779) is a separate pre-existing issue, and fixing jsgen.genSwap instead of the (simpler) approach I took here would not have helped with that VM bug.

This PR fixes 2 issues (and simplifies js code), so IMO is a net improvement; future work can fix the remaining vm issue which will probably involve unrelated code changes.

@Araq
Copy link
Member

Araq commented Feb 2, 2021

would not have helped with that VM bug.

So we need to fix both vmgen and jsgen without hacks. I fail to see the disadvantage, still better than a hack inside system.nim that stays for good.

@Araq
Copy link
Member

Araq commented Feb 16, 2021

No activity, closing. Sorry.

@ringabout
Copy link
Member

Succeeded by #23473

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.

swap silently broken for objects inside functions (js target)
4 participants