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

Polyfill table.move for 32-bit Gmod #2021

Merged
merged 1 commit into from Nov 28, 2023
Merged

Conversation

Denneisk
Copy link
Contributor

Implements table.move for 32-bit Gmod as accurately as possible, including errors, to preserve forward-compatibility. This solution is, naturally, not completely optimal.

Comment out the if statement and rename the function to compare outputs.

PrintTable( table.move( { 1, 2, 3, 4 }, 1, 4, -4 ) )
--[[ Outputs:
-4	=	1
-3	=	2
-2	=	3
-1	=	4
1	=	1
2	=	2
3	=	3
4	=	4
]]

Additionally, the documentation on the wiki is incorrect. The 4th argument, dest, is required (you can test this yourself).

@Kefta
Copy link
Contributor

Kefta commented Oct 27, 2023

LuaJIT will be upgraded on main when it's branch merge time. I don't see the need to implement this in Lua right now. Servers can do it themselves for main branch clients.

@Grocel
Copy link

Grocel commented Oct 28, 2023

LuaJIT will be upgraded on main when it's branch merge time. I don't see the need to implement this in Lua right now. Servers can do it themselves for main branch clients.

I don't think this will happen any time soon. We have been in limbo state on several awaited features for YEARS by now. So better account for things taking a while, then speculate on "soon™". Things taking time, that's how it is. I personally expect it to take at least another 3-5 years until we come to a conclusion on that matter.

So add polyfills as long they are needed for the time being, especially then they were already done for you via a PR. If you want cross branch-compatibility than do it properly. Otherwise it will just create additional unnecessary community pressure on the dev.

I have seen a lot of tip toeing around over potential braking branch-compatibility changes over the years. So at this point a "well do it your self" on a PR doing the same tip toeing is just ridiculous.

@robotboy655 robotboy655 added the Addition The pull request adds new functionality. label Oct 31, 2023
@robotboy655 robotboy655 merged commit 50fd1f8 into Facepunch:master Nov 28, 2023
Copy link

@bjcscat bjcscat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be cleaner if assert was used.

        assert(istable( sourceTbl ), "bad argument #1 to 'move' (table expected, got " .. type( sourceTbl ) .. ")" )
        assert(isnumber( from ), "bad argument #2 to 'move' (number expected, got " .. type( from ) .. ")" )
        assert(isnumber( to ), "bad argument #3 to 'move' (number expected, got " .. type( to ) .. ")" )
        assert(isnumber( dest ), "bad argument #4 to 'move' (number expected, got " .. type( dest ) .. ")" )
        if destTbl ~= nil then
            assert(istable( destTbl ), "bad argument #5 to 'move' (table expected, got " .. type( destTbl ) .. ")")
        else
            destTbl = sourceTbl
        end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Addition The pull request adds new functionality.
Projects
None yet
5 participants