-
Notifications
You must be signed in to change notification settings - Fork 71
Support for Lua 5.4+ & C++17 #111
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
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
satoren
left a comment
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.
Thank you
Lua 5.4 actually sets
nresultsinlua_resumewhich previously was hardcoded to 1. This exposes an error with the start index passed toFunctionResultProxy::ReturnValue:resultCount_ = lua_gettop(thread) + 1 - startIndexstartIndexwas set tonresult - (lua_gettop(state) + 1)nresult == 1is 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_statusinside the panic handler always returns "OK"C++17 defines an output operator for
nullptrso 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::ReturnValuecalculation that was exposed by Lua 5.4's change tolua_resume.lua_statusalways returns "OK" inside the panic handler.Enhancements:
Tests: