franko / luajit-lang-toolkit

A Lua bytecode compiler written in Lua itself for didactic purposes or for new language implementations
Other
652 stars 91 forks source link

Incorrect handling of call expression in tests #7

Closed q66 closed 10 years ago

q66 commented 10 years ago

A simple test:

local k = "foo"
k = tonumber(k) and "foo" or k
print("K", k)

This will print nil for "k". Not sure what the best solution would be here. I see that an extra "mov" instruction is being generated, incorrectly overwriting the value.

franko commented 10 years ago

Hi,

just for your information, I've been thinking about this problem since yesterday. It is not trivial to fix and the logic of the current implementation is wrong so I need some time to come up with the right solution. Probably I will be able to fix that in the next few days when I will be sure about the right approach to use.

q66 commented 10 years ago

I know it's tricky, which is why I haven't provided a patch right away :) If it was simple, I would've patched it. I'll let you know if I manage to patch it before you do.

franko commented 10 years ago

Ok, I've to go now but I can give you an hint. The method test_emit store a value based on the "store" argument if the expression is true, false or in both case. Instead of storing the value with MOV and then testing the value and JUMP we should juste use ISTC / ISFC that does the store and jump but only it is true or false.

My guess for the moment is that this should solve the problem but it needs to be verified.

Bye

franko commented 10 years ago

Hi Daniel,

I think I was able to fix the problem and I'm relatively sure that the solution is sound. The fix is in the branch github-issue-7 for the moment. I hope you will be able to test it.

q66 commented 10 years ago

thanks; I wasn't able to take a look at it as I didn't really have time through today/yesterday. I'll give it a shot.

q66 commented 10 years ago

Fix indeed works and can confirm that it seems sound. Please close the issue when you've pushed the fix to master.

franko commented 10 years ago

Fix merged in master branch.