beyond-all-reason / beyond-all-reason/RecoilEngine

Refactor C Lua API usage to avoid negative indices usage when getting values

Open
#2,250 0 comments 0 reactions 0 assignees View on GitHub
area: Lua API refactor
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.