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

LuaD enhancements #72

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

TurkeyMan
Copy link
Contributor

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

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

@JakobOvrum
Copy link
Owner

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

@TurkeyMan
Copy link
Contributor Author

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

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

if(len != T.length)
luaL_error(L, "Incorrect number of array elements: %d, expected: %d", len, T.length);

T arr;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should void-initialize this? To preserve @safety, the void-initialized path can be a specialization for element types without indirection (std.traits.hasIndirections should do the trick).

@JakobOvrum
Copy link
Owner

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
Copy link
Owner

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 :)

@TurkeyMan
Copy link
Contributor Author

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

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

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

pushMeta!(BaseClassesTuple!T[0])(L);
lua_setmetatable(L, -2);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ Is this Particular pushMeta block in any of your feature-specific branches, @TurkeyMan ? My modification to get interfaces to work touches this and I haven't been able to find it elsewhere.

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 >_<).
…Lua code to match handling of regular tables.

struct and class have uniform support for: methods, properties, member variable accessors.
Refactor common code into helpers.d
Added support for const function args.
pushMethod enhanced to support struct's aswell.
New alias syntax updates.
Pointers are opaque.
Pointers support 'nil' assignment.
'deref' property on pointers to access the actual object.
…le(T).

Added preliminary for class inheritance.
@GreatEmerald
Copy link

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

@GreatEmerald
Copy link

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

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.

4 participants