LuaD icon indicating copy to clipboard operation
LuaD copied to clipboard

LuaD enhancements

Open TurkeyMan opened this issue 9 years ago • 11 comments

Fixed static arrays. Fixed support for calling functions which receive const args. Rework struct to use userdata; conversion is not lossy, conversion should be faster, less garbage, supports more features (methods, properties). Classes improved to support fields, properties. Static type interfaces and creation of struct instances in Lua code. Default construction is supported in static type interfaces in lieu of a constructor.

TurkeyMan avatar Aug 17 '14 17:08 TurkeyMan

I'm opening this PR to discuss my changes to LuaD, and find which of them may/should be integrated back into LuaD proper.

TurkeyMan avatar Aug 17 '14 17:08 TurkeyMan

Thanks for submitting your changes; API-wise I think all of them belong in LuaD. I'm still travelling, but I'll find time to give the implementation a proper review as soon as possible (though it looked good after a quick look on a mobile device!).

JakobOvrum avatar Aug 17 '14 22:08 JakobOvrum

Cool, well I'll continue on my current trajectory then. Comprehensive support for enum's is incoming, then I'll ponder class inheritance. Let me know when you're free again, I'd like to discuss some ideas in depth.

TurkeyMan avatar Aug 18 '14 03:08 TurkeyMan

Enums are in. They marshal as strings. pushStaticTypeInterface for enum's produces a table filled with the enums as strings. 'init', and arrays 'keys' and 'values' are also created, where the 'values' array stores the actual D values of the enum's (which probably have little use in Lua, since we're treating them as strings).

TurkeyMan avatar Aug 18 '14 14:08 TurkeyMan

The bit-field functions seem completely unused ATM.


The enum handling code assumes the base type is integral, but D enums don't have to be integral:

enum E { a = "foo" }

In the current documentation setup, the respective luad.conversions.* pages are referenced from the luad.stack page where conversions are explained broadly. Thus, luad.conversions.enums should explain how enums are handled.


Converting enum values to their respective string name should probably use binary search (we can leverage std.range.SortedRange) for good worst-case complexity.


Property setters and getters don't need to work on the same types:

struct S {
    int get() @property;
    void set(double) @property;
}

I don't think the code handles such properties correctly?


For read-only fields or properties, we should push a setter that errors saying the property is read-only. I suspect the currently raised error in these cases is very confusing.


There seems to be a lot of duplication between the marshalling codes for classes and structs; maybe merge them into one module with unified documentation?

JakobOvrum avatar Aug 19 '14 22:08 JakobOvrum

Putting orthogonal changes in separate pull requests makes review and merging much easier/faster, so if you're comfortable enough with Git that it isn't too much of a hassle, please consider doing so for further additions :)

JakobOvrum avatar Aug 19 '14 22:08 JakobOvrum

The bit-field functions seem completely unused ATM.

Correct, they're a block comment with 'TODO' :) .. Just a somewhat elaborate TODO with code written.

The enum handling code assumes the base type is integral, but D enums don't have to be integral [...]

How do you come to that conclusion? I use string enums, they work. KeyValuePair(ValueType) has a templated value type, and:

        // push the value to the values array
        pushValue!(OriginalType!T)(L, e.value);

Uses OriginalType! to push the appropriately typed values.

Converting enum values to their respective string name should probably use binary search (we can leverage std.range.SortedRange) for good worst-case complexity.

I agree, hence:

// TODO: These linear lookups are pretty crappy... we can do better, but this get's us working.

Property setters and getters don't need to work on the same types [...]. I don't think the code handles such properties correctly?

Getters return auto, so they're fine. Setters can only work once we have function overloads working properly... but that's quite a big job. There is an issue in there now where the type supplied to the setter is captured from the return value of the getter, there was a TODO comment in there to capture the arg type of the setter instead, but that gets tricky with overloads.

For read-only fields or properties, we should push a setter that errors saying the property is read-only. I suspect the currently raised error in these cases is very confusing.

I agree, I haven't done it yet.

There seems to be a lot of duplication between the marshalling codes for classes and structs; maybe merge them into one module with unified documentation?

Everything is still moving, I'm nowhere near finished. I simplify the code at the end.

Putting orthogonal changes in separate pull requests makes review and merging much easier/faster, so if you're comfortable enough with Git that it isn't too much of a hassle, please consider doing so for further additions :)

If you're happy to merge them quickly, I can separate them. The problem is, my more advanced changes (and wip) permute the code I changed in the earlier/simpler feature commits. I'll end up with a growing number of annoying merge conflicts if I can't update to the merged master quickly.

TurkeyMan avatar Aug 20 '14 07:08 TurkeyMan

I'll do a proper job splitting it up into different patch sets for individual merging when I'm done. I opened this pull request so you could see what I was doing, since my changes are quite extensive, I wanted to have early warning if I start in any direction that you're not happy with.

TurkeyMan avatar Aug 20 '14 09:08 TurkeyMan

Okay, I've rebased, rebased and rebased some more the history into orthogonal PR's. Everything is squashed. It should be easier to review.

Now that I have done this, I am effectively blocked until these PR's are merged. My live test environment depends on all these commits, so I can't really make further changes until they are all merged in together. If I need to make amendments to any of these PR's for acceptance, it will be an ongoing hassle to merge them into my live working set...

TurkeyMan avatar Aug 21 '14 06:08 TurkeyMan

You forgot to also update the Makefile with the new files you added.

GreatEmerald avatar Feb 27 '18 07:02 GreatEmerald

Also, somehow this seems to break reading more complex values from Lua scripts, I keep getting the error userdata expected, got table whenever trying to either read an array of structs or a struct with complex types inside (functions, other structs that include arrays, etc.)

GreatEmerald avatar Mar 03 '18 11:03 GreatEmerald