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

Comment in lgc.h wrongly describes the GC invariant? #1282

Closed
dubiousjim opened this issue Jun 2, 2024 · 1 comment · Fixed by #1288
Closed

Comment in lgc.h wrongly describes the GC invariant? #1282

dubiousjim opened this issue Jun 2, 2024 · 1 comment · Fixed by #1288

Comments

@dubiousjim
Copy link

This line describes the GC invariant as being the white objects can never point to black ones. Reading that made the GC code and other comments bewildering, until I realized that the referenced comment is probably just backwards. Isn't the invariant instead that black objects can never point to white ones? Before an object can be promoted from gray to black, all the collectable objects it points to must themselves be darkened (from white to gray, or left as gray or black if they've already been so marked).

@zeux
Copy link
Collaborator

zeux commented Jun 7, 2024

Yeah this comment is backwards, this is from original Lua 5.1 source. Recent Lua source contains a correct comment (https://github.com/lua/lua/blob/master/lgc.h#L17-L28) that also applies to Luau implementation except that "moreover" statement would need to be tweaked.

lgc.cpp contains a full description of the scheme which correctly spells the invariant: https://github.com/luau-lang/luau/blob/master/VM/src/lgc.cpp#L58

vegorov-rbx pushed a commit that referenced this issue Jun 7, 2024
The comment gave an incorrect (reversed) version of the invariant, which
could be confusing for people who haven't read the full description in
lgc.cpp.

Unfortunately this change is difficult to flag.

Fixes #1282.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants