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

✨ Test and fix #1315 #1369

Merged
merged 7 commits into from
Jun 24, 2022
Merged

✨ Test and fix #1315 #1369

merged 7 commits into from
Jun 24, 2022

Conversation

ShepherdSoasis
Copy link
Contributor

This pull request formally verifies that #1315 is fixed by adding it to the tests. It tests all the various permutation of collection-returns through Lua. Lua 5.2 specifically has the ipairs method that calls the custom __ipairs metamethod in Lua, and that is where the failure is detected as it did not keep the original collection (in the bug report's case, a std::vector). This failure does not seem to be present on Lua 5.1, 5.3 and 5.4 because the custom __ipairs metamethod was deprecated and removed immediately after its introduction in 5.2.

There is also some general fixes to update the behavior of some of the other tests and bring them in line with others, utilizing Catch2 to do so, which generally makes this a fairly messy commit. More commits will be added as this fix is verified for the users.

— 🛠 Update SOL_IS_(DEFAULT_)ON/OFF usage to be more idiomatic and less confusing (add SOL_RAW_* alternatives as well)
— 💚 Re-check CI
— 👷‍♀️ Add missing header from ebco.hpp
— Lua 5.1, 5.2, and 5.3 seemed to have a deficiency in which the stack thread space variable (given as a lua_State*) would die before everything referencing it would be properly dead. This made holding a reference for keep-alive purposes impossible to maintain. Therefore, we retrieve the main thread to keep it alive.
@ShepherdSoasis
Copy link
Contributor Author

We have confirmed this fix works for the folks we were working with, and it additionally restores the tests to a working state (it does not expand the testing matrix in CI).

OK to merge?

@ThePhD ThePhD self-assigned this Jun 24, 2022
@ThePhD ThePhD added the Bright.Future Funny, believing there's a future here, innit? label Jun 24, 2022
@ThePhD ThePhD assigned ThePhD and unassigned ThePhD Jun 24, 2022
@ThePhD ThePhD added Thank.You Bug.Derp They don't call me The Phantom Derpstorm for no reason. labels Jun 24, 2022
@ThePhD ThePhD added this to the Bugs milestone Jun 24, 2022
@ThePhD ThePhD merged commit 3b97af0 into develop Jun 24, 2022
@ThePhD ThePhD deleted the fix/#1315 branch June 24, 2022 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bright.Future Funny, believing there's a future here, innit? Bug.Derp They don't call me The Phantom Derpstorm for no reason. Thank.You
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants