beyond-all-reason / beyond-all-reason/RecoilEngine
Refactor C Lua API usage to avoid negative indices usage when getting values
- Dominant language
- C++
- Stars
- 679
- Forks
- 290
- Avg merge
- 3d 2h
- Merged PRs (30d)
- 40
Description
### Problem
- Read chapter 3.1 in [lua 5.1 reference manual](https://www.lua.org/manual/5.1/manual.html) regarding stack indices
- Find `luaL_checkint` usage in these [3 synced callouts](https://github.com/beyond-all-reason/RecoilEngine/blob/47cc5a9657a0c3241c95f06dd17047ce4c119d89/rts/Lua/LuaSyncedRead.cpp#L2324-L2390)
- Notice unnecessary (and even wrong) assignment of variables based on negative indices usage (e.g. team1 and team2 are swapped in `AreTeamsAllied`)
### Proposed solution
- Fix these 3 linked callouts to use positive indices in `luaL_checkint`
- Find other similar problematic cases, if any, fix them
- Notice this issue concerns only with fetching function arguments (negative indices are expected to be used in other places, for example `lua_rawseti`). Affected cases would look something like `luaL_(L, -n)`, there might be other problematic cases I haven't mapped here.
### Remarks
Notice there's nothing wrong with using negative indices, except when doing so makes it harder to reason about and maintaining the code.
Example, suppose a function `Spring.Do(team1)`, if using negative indices: `team1 = fetch(L, -1)`. If in the future `Spring.Do` is changed to accept a new argument `foo` then the code needs to significantly change: `team1 = fetch(L, -2); foo = fetch(L, -1)`, as opposed to just adding a single new line, e.g. `foo = fetch(L, 2) // note 2 means clearly second argument`.
There should be no scenario where the usage above makes sense from a maintainability standpoint, except for way complex scenarios (dynamic argument count, etc).
The 3 cases I linked are such problematic cases.
Contributor guide
Research direction
Read the Lua 5.1 stack-index section, then inspect the three linked callouts in rts/Lua/LuaSyncedRead.cpp around lines 2324-2390. Search for similar luaL_check/opt(L, -n) argument fetches, update only cases where positive indices are appropriate, and verify that expected negative-index uses such as lua_rawseti remain unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, lua
- Domain
- game-dev
- Issue type
- Refactor
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100