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

app/lua53: catch up to lua 5.3.6 #3415

Merged
merged 1 commit into from
Dec 22, 2022
Merged

Conversation

nwf
Copy link
Member

@nwf nwf commented Mar 29, 2021

Diff reduction to 5.3.6, mostly guided by upstream.

Tested lightly using a subset of the expect framework; enough to verify that LFS continues to function, at least!

  • This PR is for the dev branch rather than for the release branch.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/*.

@nwf nwf requested a review from TerryE March 29, 2021 00:26
@nwf
Copy link
Member Author

nwf commented Apr 14, 2021

Nobody here or #3414?

Copy link
Member

@HHHartmann HHHartmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit concerned about the luaC_objbarrier calls, as it seems they belong to GC and we have some modifications in that area.
The rest I guess is just from upstream and I guess they know what they are doing, so that's OK.

@nwf what exactly do you mean by not checking the "I tested" checkbox. Did you run the code on ESP?
Did you run the Lua test suite?

Do we need to test GC in combination with our RO elements for this?
Maybe @TerryE can help out.

@nwf
Copy link
Member Author

nwf commented Dec 27, 2021

When I said "mostly guided by upstream", I meant that most of this was https://www.lua.org/work/diffs-lua-5.3.5-lua-5.3.6.html . The objbarriers are from that, FWIW.

Adapt https://www.lua.org/work/diffs-lua-5.3.5-lua-5.3.6.html to
NodeMCU.  Mostly a straight application, but some small tweaks were
required and, in lundump.c, some changes were elided and some additional
diff reduction applied, as we have heavily diverged from upstream.
@nwf nwf force-pushed the 202103-lua53-dot6 branch from e6f2090 to ef256de Compare December 28, 2021 06:34
@nwf
Copy link
Member Author

nwf commented Dec 28, 2021

Ah ha. After a day of debugging I have finally caught my mistake in this. Of course it was in the lundump.c bit where we've diverged from upstream. LoadFunction does not always (but does sometimes!) return its second argument, so it's important to re-assign, as in p[i] = LoadFunction(S, p[i], f->source);.

This now passes my expect harness's tests, modulo some local network shenanigans. But LFS got plenty of exercise throughout the run. :)

@nwf
Copy link
Member Author

nwf commented Dec 28, 2021

Will merge after #3479 lands.

@nwf nwf added this to the 2022 rel 1 milestone Jan 2, 2022
@marcelstoer
Copy link
Member

Will merge after #3479 lands.

Oh, that was a year ago 😄 Will merge now.

@marcelstoer marcelstoer merged commit fc771cd into nodemcu:dev Dec 22, 2022
marcelstoer pushed a commit that referenced this pull request Feb 25, 2024
Adapt https://www.lua.org/work/diffs-lua-5.3.5-lua-5.3.6.html to
NodeMCU.  Mostly a straight application, but some small tweaks were
required and, in lundump.c, some changes were elided and some additional
diff reduction applied, as we have heavily diverged from upstream.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants