-
Notifications
You must be signed in to change notification settings - Fork 71
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
Support for Lua 5.4+ & C++17 #111
base: main
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request introduces support for Lua 5.4+ and C++17. The main changes include updating the coroutine handling to correctly manage the number of results returned by File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Flamefire - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
The couroutine result index is always 1, not dependent on the number of results which is not available in < 5.4. Errors during GC are shown as warnings in 5.4+
A change in Lua 5.4.4 resets the Lua status to OK before calling the panic handler. This affects a test checking for an out-of-memory exception. Check the message string instead for something memory related which is "good enough".
31bacc7
to
a808e47
Compare
Lua 5.4 actually sets
nresults
inlua_resume
which previously was hardcoded to 1. This exposes an error with the start index passed toFunctionResultProxy::ReturnValue
:resultCount_ = lua_gettop(thread) + 1 - startIndex
startIndex
was set tonresult - (lua_gettop(state) + 1)
nresult == 1
is hardcoded:startIndex = -lua_gettop(state)
resultCount_ =
lua_gettop(thread) + 1 + lua_gettop(state)` which doesn't hold (at least for 5.4)Finally in Lua 5.4.4+
lua_status
inside the panic handler always returns "OK"C++17 defines an output operator for
nullptr
so we need to remove the custom one.GCC 13 warns about the "dangling references" returned by
get<unique_ptr&>
which is a false positive in this case.Summary by Sourcery
Update the library to support Lua 5.4+ and C++17.
Bug Fixes:
FunctionResultProxy::ReturnValue
calculation that was exposed by Lua 5.4's change tolua_resume
.lua_status
always returns "OK" inside the panic handler.Enhancements:
Tests: