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

Replace LuaVariant with std::variant #3981

Merged
merged 3 commits into from
Mar 7, 2022

Conversation

ranisalt
Copy link
Member

@ranisalt ranisalt commented Feb 28, 2022

Pull Request Prelude

Changes Proposed

Replaces custom LuaVariant class with std::variant with the same types. This reduces memory size of each instance from 56 bytes to 40 bytes, and allows for some code cleaning.

Issues addressed:
Closes #1481

src/luavariant.h Outdated Show resolved Hide resolved
@MillhioreBT
Copy link
Contributor

MillhioreBT commented Feb 28, 2022

I agree with using std::variant, but I'd love for it to be wrapped in a structure with the following methods:
is get set
per example:
isNumber setTargetPosition, getString

@ramon-bernardo
Copy link
Contributor

I saw an inconsistency, some names as var and others as variant, worry?

src/luavariant.h Outdated Show resolved Hide resolved
@ranisalt
Copy link
Member Author

ranisalt commented Mar 1, 2022

I saw an inconsistency, some names as var and others as variant, worry?

A rewrite of the code to fix this inconsistent naming is long overdue, but should be a separate PR.

@MillhioreBT
Copy link
Contributor

I think you just have to add a TAB to the content of the class {...} and it will be ready

src/spells.cpp Outdated Show resolved Hide resolved
@Kamenuvol
Copy link
Contributor

Issues addressed: Closes #1482

is this right?

@ranisalt
Copy link
Member Author

ranisalt commented Mar 6, 2022

is this right?

No, it should be #1481

Agh, off by one errors...

@EPuncker EPuncker added the enhancement Increase or improvement in quality, value, or extent label Mar 6, 2022
@EPuncker EPuncker merged commit 1cef0ea into otland:master Mar 7, 2022
@ranisalt ranisalt deleted the luavariant-std-variant branch March 7, 2022 23:26
Codinablack pushed a commit to Codinablack/forgottenserver that referenced this pull request Apr 5, 2022
EPuncker pushed a commit to EPuncker/forgottenserver that referenced this pull request May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Increase or improvement in quality, value, or extent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Union on LuaVariant
5 participants