-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[no sq] Safety and other fixes #15251
Conversation
6eaf0f7
to
d019d69
Compare
@@ -231,143 +249,143 @@ void TestVoxelArea::test_index_xyz_all_pos() | |||
VoxelArea v1; | |||
UASSERTEQ(s32, v1.index(156, 25, 236), 155); | |||
|
|||
VoxelArea v2(v3s16(756, 8854, -875), v3s16(-147, -9547, 669)); | |||
UASSERTEQ(s32, v2.index(156, 25, 236), 1267138774); | |||
VoxelArea v2(v3s16(-147, -9547, -875), v3s16(756, 8854, 669)); |
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.
note: all of these positions were not sorted, causing garbage results
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.
Looks good, works. Will approve after the few comments were addressed.
7ca1551
to
bc3a5fc
Compare
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.
(Comments for 1st commit.)
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.
(2nd commit.)
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.
3rd
static void set_null(lua_State *L); | ||
// Clear the pointer in the ObjectRef (at -1). | ||
// Throws an fatal error if the object pointer wasn't `expect`. | ||
static void set_null(lua_State *L, void *expect); |
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.
Why void ptr?
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.
🤷
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.
Looks good. 👍
(Didn't test.)
In particular this validates the edges of VoxelArea and fixes all the nonsense tests uncovered by it.
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.
(Haven't tested much.)
also add override keyword and fix overflow() behavior
concrete problem: the getEmergeThread safety check was missing there
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.
Re-read the code and checked a few singleplayer worlds with Lua mapgens. No issue found. No new valgrind error appears.
fixes #14435
fixes https://github.com/orgs/minetest/discussions/111 (internal)
fixes https://github.com/orgs/minetest/discussions/108 (internal)
fixes https://github.com/orgs/minetest/discussions/89 (internal)
To do
This PR is Ready for Review.
How to test
see linked discussions