Conversation
📝 WalkthroughWalkthroughMap generation was refactored to avoid global Changes
Sequence Diagram(s)sequenceDiagram
participant C as C (generate_map)
participant Lua as Lua mapgen script
participant Builder as maproombuilder
participant Map as Map object
C->>Lua: call map generation (lua_call)
Lua->>Map: local map = create_map(...)
Lua->>Builder: module.load_textures(map)
Builder->>Builder: load_decor_textures(map) / load_special_tiles(map)
Builder-->>Lua: textures added to map
Lua->>Map: populate rooms / monsters / traps / chests
Lua-->>C: return map (push userdata onto stack)
C->>C: luaL_checkmap(L, -1) -> obtain Map*
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
map no longer needs to be a global variable.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/map_lua.c (1)
707-713:⚠️ Potential issue | 🔴 CriticalValidate Lua return before dereferencing map pointer.
Line 712 uses unchecked
lua_touserdata(L, -1)and line 718 immediately dereferences it without validation. If the script returns nil/non-pointer (or unexpected extra returns withLUA_MULTRET), this crashes.🔧 Proposed fix
- result = lua_pcall(L, 0, LUA_MULTRET, 0); + result = lua_pcall(L, 0, 1, 0); if (result) { fatal("Failed to run script: %s\n", lua_tostring(L, -1)); } - Map *map = lua_touserdata(L, -1); + Map *map = luaL_checkmap(L, -1);
luaL_checkmap()(defined at line 72) validates the type and null-checks before returning, consistent with how it's used throughout the rest of the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/map_lua.c` around lines 707 - 713, The code calls lua_pcall(L, 0, LUA_MULTRET, 0) and then directly uses lua_touserdata(L, -1) which can return nil or a wrong type; replace the unchecked lua_touserdata usage with the type- and null-checked helper luaL_checkmap(L, -1) (the existing function defined at line 72) to validate the returned value before dereferencing the Map pointer, and keep the fatal/log behavior consistent if luaL_checkmap reports an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/map_lua.c`:
- Around line 707-713: The code calls lua_pcall(L, 0, LUA_MULTRET, 0) and then
directly uses lua_touserdata(L, -1) which can return nil or a wrong type;
replace the unchecked lua_touserdata usage with the type- and null-checked
helper luaL_checkmap(L, -1) (the existing function defined at line 72) to
validate the returned value before dereferencing the Map pointer, and keep the
fatal/log behavior consistent if luaL_checkmap reports an error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71e21407-a351-4ae9-854f-aa7bcf6d5fd0
📒 Files selected for processing (4)
data/mapgen.luadata/maproombuilder.luadata/menumapgen.luasrc/map_lua.c
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/map_lua.c (1)
707-713: Harden the Lua contract by requesting exactly one return value.The generator scripts (mapgen.lua and menumapgen.lua) currently return only a map value. However, using
LUA_MULTRETmeans the code depends on the map being the last returned value. Requesting exactly one result makes the interface explicit and prevents breakage if a script accidentally returns extra values.Proposed hardening diff
- result = lua_pcall(L, 0, LUA_MULTRET, 0); + result = lua_pcall(L, 0, 1, 0); if (result) { fatal("Failed to run script: %s\n", lua_tostring(L, -1)); } Map *map = luaL_checkmap(L, -1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/map_lua.c` around lines 707 - 713, The call to lua_pcall currently uses LUA_MULTRET which allows any number of return values and relies on the map being the last; change the pcall invocation in this block (lua_pcall) to request exactly 1 result instead of LUA_MULTRET so the script contract is explicit, then keep using luaL_checkmap(L, -1) to fetch that single return value; ensure the existing error path (fatal with lua_tostring) remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/map_lua.c`:
- Around line 707-713: The call to lua_pcall currently uses LUA_MULTRET which
allows any number of return values and relies on the map being the last; change
the pcall invocation in this block (lua_pcall) to request exactly 1 result
instead of LUA_MULTRET so the script contract is explicit, then keep using
luaL_checkmap(L, -1) to fetch that single return value; ensure the existing
error path (fatal with lua_tostring) remains unchanged.
1 participant
map no longer needs to be a global variable.
Summary by CodeRabbit