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

Support for Lua 5.4+ & C++17 #111

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Jan 5, 2025

Lua 5.4 actually sets nresults in lua_resume which previously was hardcoded to 1. This exposes an error with the start index passed to FunctionResultProxy::ReturnValue:

  • The number of results is calculated as resultCount_ = lua_gettop(thread) + 1 - startIndex
  • startIndex was set to nresult - (lua_gettop(state) + 1)
  • as nresult == 1 is hardcoded: startIndex = -lua_gettop(state)
  • So it follows: resultCount_ = lua_gettop(thread) + 1 + lua_gettop(state)` which doesn't hold (at least for 5.4)
  • In fact the start index for results is constantly 1, index 0 is (likely) the success state (bool)

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:

  • Fix an error in the FunctionResultProxy::ReturnValue calculation that was exposed by Lua 5.4's change to lua_resume.
  • Handle the change in Lua 5.4.4+ where lua_status always returns "OK" inside the panic handler.

Enhancements:

  • Improve coroutine support by enabling mixed yielding and receiving of parameters.

Tests:

  • Add tests for mixed yielding and receiving of parameters in coroutines.
  • Update tests to account for Lua 5.4.4+ panic handler behavior.

Copy link

sourcery-ai bot commented Jan 5, 2025

Reviewer's Guide by Sourcery

This 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 lua_resume in Lua 5.4+, handling a change in error status in Lua 5.4.4+ within the panic handler, removing a custom output operator for nullptr due to its definition in C++17, and addressing a false positive dangling reference warning in GCC 13.

File-Level Changes

Change Details Files
Updated coroutine handling to support Lua 5.4+
  • Modified lua_resume result handling to account for Lua 5.4+ changes in the number of returned results.
  • Added tests for mixed yielding and receiving parameters in coroutines.
  • Corrected the calculation of resultCount_ and startIndex in FunctionResultProxy::ReturnValue to handle Lua 5.4+ behavior where nresults is set by lua_resume.
  • Ensured all results are on top of the thread stack after lua_resume.
test/test_04_lua_function.cpp
include/kaguya/detail/lua_function_def.hpp
Handled Lua 5.4.4+ panic handler error status change
  • Added a check for LuaUnknownError and its message in the allocation error test to handle the case where lua_status returns "OK" inside the panic handler in Lua 5.4.4+.
test/test_06_state.cpp
Removed custom output operator for nullptr
  • Removed the custom output operator for nullptr as it is now defined in C++17.
test/test_util.hpp
Addressed GCC 13 dangling reference warning
  • Added a compiler flag -Wno-dangling-reference to suppress the false positive warning in GCC 13 about dangling references returned by get<unique_ptr&>.
CMakeLists.txt
Added support for Lua 5.4.7 in CI
  • Added Lua 5.4.7 to the test matrix in the CI workflow file.
.github/workflows/test.yml
Updated C++ standard to C++17
  • Modified CMakeLists.txt to use C++17.
CMakeLists.txt
Minor updates and fixes
  • Added #include <limits>.
test/test_01_primitive.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a 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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

include/kaguya/detail/lua_function_def.hpp Outdated Show resolved Hide resolved
include/kaguya/detail/lua_function_def.hpp Show resolved Hide resolved
test/test_06_state.cpp Show resolved Hide resolved
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".
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.

1 participant