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

LUA_PACK_VALUE setXvalue side effect order fix #71

Merged
merged 1 commit into from Oct 13, 2015
Merged

LUA_PACK_VALUE setXvalue side effect order fix #71

merged 1 commit into from Oct 13, 2015

Conversation

szakharchenko
Copy link
Contributor

No description provided.

…acros to match the original (and expected) one.
@bogdanm
Copy link
Member

bogdanm commented Oct 12, 2015

Hi,

Is this meant to fix #69?

Thanks,
Bogdan

@szakharchenko
Copy link
Contributor Author

No, this is meant to make the two implementations (LUA_PACK_VALUE/!LUA_PACK_VALUE) agree. Vanilla Lua 5.1.4 shouldn't care about the side effect order in set*macros. Seems like the emergency GC patch, or some other patch in eLua, breaks this. Otherwise, why would it bother to reorder the side effects, e.g:

--- a/src/lobject.h
+++ b/src/lobject.h

...

#define setnvalue(obj,x) \
-{ TValue *i_o=(obj); i_o->value.n=(x); i_o->tt=LUA_TNUMBER; }
+{ lua_Number i_x = (x); TValue *i_o=(obj); i_o->value.n=i_x; i_o->tt=LUA_TNUMBER; }

However, the NaN value packing patch is based on the old version of the same code, which now (thanks to other patches) works incorrectly, and needs to be adjusted to match the EGC patch behavior.

When I asked about this on the list, Roberto replied it would be better to use proper temporary variables for this. See the emails for how to reproduce the issue I saw. I may create a proper issue if/when I have time if you need it.

@bogdanm
Copy link
Member

bogdanm commented Oct 13, 2015

Thanks a lot for the fix!

bogdanm added a commit that referenced this pull request Oct 13, 2015
LUA_PACK_VALUE  setXvalue side effect order fix
@bogdanm bogdanm merged commit be3e050 into elua:master Oct 13, 2015
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.

None yet

2 participants