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

Added support for enum's. #74

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

Conversation

TurkeyMan
Copy link
Contributor

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
Copy link
Contributor Author

Just the enum patch.

@TurkeyMan
Copy link
Contributor Author

Removed the dead code.

@TurkeyMan
Copy link
Contributor Author

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

@JakobOvrum
Copy link
Owner

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.

@TurkeyMan
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@aubade
Copy link
Contributor

aubade commented Sep 27, 2014

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.

@TurkeyMan
Copy link
Contributor Author

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
Copy link
Contributor Author

@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.

@aubade
Copy link
Contributor

aubade commented Sep 27, 2014

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

@TurkeyMan
Copy link
Contributor Author

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.

@aubade
Copy link
Contributor

aubade commented Nov 8, 2014

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.

@TurkeyMan
Copy link
Contributor Author

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.

@aubade
Copy link
Contributor

aubade commented Nov 9, 2014

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.

@TurkeyMan
Copy link
Contributor Author

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...

@aubade
Copy link
Contributor

aubade commented Nov 9, 2014

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?

@TurkeyMan
Copy link
Contributor Author

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.

@aubade
Copy link
Contributor

aubade commented Nov 9, 2014

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.

@JakobOvrum
Copy link
Owner

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...

@TurkeyMan
Copy link
Contributor Author

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.

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 >_<).
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.

3 participants