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

Fix integer sizing in Enum #902

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

googleben
Copy link

Fixes #888.

Pretty straightforward changes - mainly just converted Enum.m_value to a union and wrote conversions where needed. Important bits:

  • Two constructors still only take int32_t which theoretically isn't wide enough, but they're currently only called using numbers from Lua which aren't precise enough either. Could be fixed, but a cursory scan didn't turn up anything currently in the game that's large enough to need more than an int32, so I didn't worry about it
  • I went ahead and switched to all signed numbers. I haven't seen anything that seems unsigned, but there's quite a lot of obviously negative numbers, so this seems reasonable to me
  • I modified the EnumInt function exposed to Lua to return a correctly sized int. This isn't strictly necessary and it could just be an Int64 if that's preferred
  • For testing I created enums for every size that each had values of 0, 1, 2, -1, -2, and the max and min values for the type. Everything seemed to work correctly, except of course that Lua can't represent max int64_t with its native numbers
  • I made sure all the Enum constructors work correctly from Lua

Let me know if there's any changes I need to make or other information you need!

@WSSDude
Copy link
Collaborator

WSSDude commented Nov 9, 2023

Everything seemed to work correctly, except of course that Lua can't represent max int64_t with its native numbers

LuaJIT provides ability to do this, we have some converter for 64-bit values. They have to be suffixed, we have it hacked a bit that it creates specific lua string and executes it to get correct type. There is probably better way but this is what we have and do.

@WSSDude
Copy link
Collaborator

WSSDude commented Nov 9, 2023

ToLua conversion:

if constexpr (std::is_integral_v<T> && sizeof(T) == sizeof(uint64_t))

FromLua (ToRED) conversion:

else if (IsLuaCData(aObject))

@WSSDude
Copy link
Collaborator

WSSDude commented Nov 9, 2023

If it doesnt work for some reason, it should be looked at, we need to be able to work with 64 bit numbers from Lua in some way.

If on the Lua side you tried to just put some big number, try to add that ll/ull suffix.

If this fails getting to Lua properly, I would have a look if it doesn't fail to convert properly, but I would guess this is called somewhere down the line and it should work, that it is a suffix issue on Lua side (you didn't include it so it didnt know what to do).

@WSSDude
Copy link
Collaborator

WSSDude commented Nov 9, 2023

I did not have a look at the changes btw, would like to do so later... Think this needs thorough look, need to make sure you do static conversions correctly (that may be the biggest issue with such solution). Just skimmed through it, saw some safe sets/gets...

Just reacting to what you wrote now more or less, dont take it as a review :P

@googleben
Copy link
Author

Appending ll did the trick - didn't realize I could do that lol. I think that also means I don't have any more concerns about retyping those 2 constructors to use int64_t, so two birds with one stone. I'll double check that and retest it when I get a chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect int handling in Enum class
2 participants