LuaD icon indicating copy to clipboard operation
LuaD copied to clipboard

Added support for enum's.

Open TurkeyMan opened this issue 10 years ago • 21 comments

Enums are held in Lua as strings. No support for enum values that are not valid keys. No support for bitfields (yet?). Conversion function performance could be improved (linear search >_<).

TurkeyMan avatar Aug 21 '14 05:08 TurkeyMan

Just the enum patch.

TurkeyMan avatar Aug 21 '14 05:08 TurkeyMan

Removed the dead code.

TurkeyMan avatar Sep 07 '14 05:09 TurkeyMan

Are there any remaining problems with this patch? I thought you said you were happy with it the other night?

TurkeyMan avatar Sep 13 '14 01:09 TurkeyMan

I'm adding tests to it, and the static interface doesn't make a whole lot of sense when enums are strings. Consider the following correct test:

    enum E { Key0, Key1 }
    pushStaticTypeInterface!E(L);
    lua_setglobal(L, "E");

    unittest_lua(L, `
        assert(type(E) == "table")
        assert(E.init == "Key0")
        assert(E.Key0 == "Key0")
        assert(E.Key1 == "Key1")
        assert(#E.keys == 2)
        assert(E.keys[1] == "Key0")
        assert(E.keys[2] == "Key1")
        assert(#E.values == 2)
        assert(E.values[1] == 0)
        assert(E.values[2] == 1)
`);

I think it's pretty unintuitive that E.Key0 is just Key0. Either static interfaces make very little sense for string enums, or this shows that enums should really be userdata. I think it's probably the latter.

JakobOvrum avatar Sep 16 '14 06:09 JakobOvrum

I agree with your view. And I also suggest the latter, but we can live with it as is for the moment I think. I do need the values array, since there's no other way to access the values in lua. the keys array just follows beside values. I thought about also populating the keys/values tables with string keys for each key too, in addition to the numeric array...

TurkeyMan avatar Sep 16 '14 06:09 TurkeyMan

For clarity, the reason I created E.Key0 == "Key0" is that if you use E.Key0 in your lua code instead of string literals, it will silently port to userdata when it is written...

TurkeyMan avatar Sep 16 '14 06:09 TurkeyMan

Is this blocked on anything in particular? Anything I can do to help?

TurkeyMan avatar Sep 26 '14 16:09 TurkeyMan

I'd like to offer any assistance I could give too. Manu's patch suite will let me get one of the biggest hacks out of my project's scripting API.

aubade avatar Sep 27 '14 06:09 aubade

I have more in the pipeline, but I need to get this sequence of patched through before I push more. The next set start building on top of this stuff, particularly the struct patch.

TurkeyMan avatar Sep 27 '14 06:09 TurkeyMan

@aubade If you want to test my patches with your stuff? I'm only testing against my own environment. Also, I get the feeling it could do with more unittests if you're up for helping out there.

TurkeyMan avatar Sep 27 '14 07:09 TurkeyMan

Sure thing. Can I just pull from your repository's master branch?

aubade avatar Sep 27 '14 09:09 aubade

Probably best approach is to fork my repo, and then work against each of my feature branches individually. Then you can create PR to me, and that will flow through to my existing PR's to mainline.

So if you want to try each of the features in my feature branches individually against your own project, and make any fixes you need to work with your own code, then PR for each one back to me. Also feel free to add any tests, I've spent a lot of time on this so far, and I just haven't had time to think about comprehensive tests.

TurkeyMan avatar Sep 27 '14 12:09 TurkeyMan

Really sorry it took so long to finally get to this but I've started playing with your code, @TurkeyMan. Initial results are really promising at the moment, but I'm having a regression with some struct properties triggering a "no setters?! shouldn't be here..." assert. I'll try and get a pared down testcase for this.

And a short list of things that conversion currently barfs on for me: -interfaces -import statements inside a struct/class but not inside a function body. -std.bitmanip.BitArray -tuples-of-structures as generated by std.typetuple.staticMap, e.g.

import std.typetuple;

template arrayOf (T) {
    alias arrayOf = T[];
}
struct A(T...) {
   staticMap!(arrayOf, T) b;
}

The last one is a bit nasty because even @noscript won't stop the compile from erroring out.

aubade avatar Nov 08 '14 16:11 aubade

Let me know when you isolate the struct property issue. I've had good results in all my cases. I think I have a fix for the module thing in another PR. I broke it into independent PR's for simplicity.

The other 3 you found are cases I never considered. Please feel free to create PR's for those cases of yours too.

TurkeyMan avatar Nov 08 '14 22:11 TurkeyMan

Alright. Pretty minimal testcase to trigger the "no setters?!" error:

import luad.all;
import std.stdio;

pragma(lib, "lua5.1");

struct A
{
    float a;

    @property {
        float c () {
            return float.init;
        }

        float c (float newval) {
            return float.init;
        }
    }

    @property {
        float d () {
            return float.init;
        }
        float d (float newval) {
            return float.init;
        }
    }


}
A func (A test) {
    writeln(test.c);

    return test;
}

void main() {

    auto state = new LuaState();

    state.openLibs();

    state["testout"] = &func;
}

And interfaces weren't too hard to implement--I'll have a pull request ready to file soon. The other two, however, are a little bit beyond my current knowledge of D's metaprogramming.

BitArray seems to be failing because it has member functions called init that LuaD's trying to use like properties, and I'm not sure how to check for this case. The Tuple issue, I haven't the foggiest idea even what's going on for; the error given is along the lines of "no property '_tuplename_field_<x>' for type 'TupleContainingStruct'" for every element in the tuple.

aubade avatar Nov 09 '14 01:11 aubade

I think the problem with your setters is that they don't return void. setters are supposed to return void... (no?)

The others should be straight forward, I'll check it out when I have some time. But they're unrelated to this (enum) pull request...

TurkeyMan avatar Nov 09 '14 01:11 TurkeyMan

Non-void setters do work and useful for cases like

auto variable = struct.property = 1;

And sorry, should I delete the comments and move them to the right PRs?

aubade avatar Nov 09 '14 01:11 aubade

Is it idiomatic? I've never seen it anywhere... why isn't that the standard?

It's better to comment in the appropriate place, but it doesn't matter now.

TurkeyMan avatar Nov 09 '14 01:11 TurkeyMan

I'm not sure why it isn't more common; I found it out myself through experimentation when a void setter couldn't pass a value transitively, so I'm guessing it's an intentional thing even if it's not explicitly mentioned.

DSFML ( https://github.com/Jebbs/DSFML ) does use non-void setters too, so I'm at least not the only person in the world who does it.

aubade avatar Nov 09 '14 01:11 aubade

DSFML ( https://github.com/Jebbs/DSFML ) does use non-void setters too, so I'm at least not the only person in the world who does it.

I think non-void setters should be used whenever the getter operation is more or less free. This is not always the case, so void setters often make sense, too.

BitArray seems to be failing because it has member functions called init that LuaD's trying to use like properties, and I'm not sure how to check for this case.

It should be fixed in Phobos. Naming a member init is asking for trouble; I think it should be illegal in the language. If LuaD could work around it easily, then sure, but I don't think it would be worth spending a lot of effort trying to work around it when LuaD is not the problem.

Is this blocked on anything in particular? Anything I can do to help?

I'm not sure why a temporary interface should be pushed to LuaD. It's knowingly not as good as it can be and just breaks code later :/

Most of all though, there needs to be tests. In the preliminary tests I wrote for this patch (why am I the one doing it??), there were basic errors in the stack manipulating code that were easily fixed by running some basic tests...

JakobOvrum avatar Nov 27 '14 11:11 JakobOvrum

Most of all though, there needs to be tests.

Yes, this would really help.

In the preliminary tests I wrote for this patch (why am I the one doing it??)

Because I've invested a lot (multiple weeks) of time into this suite of patches. Literally as much as I could spare. I need help, and it's kinda your project. I would appreciate help fleshing out some quality test cases, from whoever would like to see this stuff moved along. Also, when I started this, if you recall, I wasn't able to build+run the tests for whatever reason it was (I don't remember), so I just didn't do them as I was in progress.

I have quite some more work to do here too, but I don't want to continue until at least the struct/class patch is accepted, otherwise I start building on shaky ground. I suspect the struct/class review process may reveal some changes, so it's a waste of time to build on top of that before it's accepted.

there were basic errors in the stack manipulating code that were easily fixed by running some basic tests...

Really? Are your tests available? I'll gladly accept PR's for any tests into my fork, which I can aggregate into my set of PR's.

TurkeyMan avatar Nov 27 '14 13:11 TurkeyMan