-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add support for user defined attributes / manual overload resolution #71
base: master
Are you sure you want to change the base?
Conversation
Modifies class registration, adding support for the usage of UDAs that define alternate method names. Also adds a NoScript attribute that prevents the method from being registered.
DMD Issue 11678
I've been toying with the idea of supporting overloads transparently. The function call shim would need to be enhanced to try and match the args that were received from the Lua call against the set of overloads, and call through to the appropriate overload if a match is found. |
Given that Lua's and D's type systems don't overlap all that well. I'm going to personally vote for manual overload resolution, at least by default. |
I thought about doing automatic overload resolution but couldn't think of a way to do it without a runtime cost. You could handle overloads either in Lua or D but either way you'd have to check the parameter types and count in order to call the appropriate function. I've fixed the unit tests added by this PR and attempted to fix some previously existing vararg related unit test failures (albeit without much success, since I can't reproduce them locally). |
Yeah, I think I'd do the parameter checks on the D side, it should be a little bit faster. @aubade: My pull to make struct's raw userdata make the type system overlap much better, and I'm about to commit comprehensive support for enum's. |
@@ -24,6 +24,22 @@ import luad.c.all; | |||
|
|||
import luad.stack; | |||
|
|||
// User defined attributes that can be placed on functions in order to change the binding behavior | |||
// Prevents this function from being registered | |||
enum NoScript = "NoScript"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use an empty struct here instead of a string. If say, PyD decided to use exactly the same method, there wouldn't be any way for the user to discriminate between Lua and Python scripts when marshalling the given method.
Also, clever uses of strings as attributes in other programs could lead to difficult-to-find bugs when used with LuaD:
alias strings = TypeTuple!("foo", "NoScript", "bar"); // happens to contain the string "NoScript"
void foo() @(strings) // This function is now NoScript for LuaD
{
}
Using instances of structs is just more hygienic in general.
I posted some implementation concerns in line comments. I like the Lua-agnostic attribute names and I think the It's not entirely clear whether attribute identifiers should be capitalized or not. I'm leaning towards non-capitalization, because as discussed in the original UDA thread, it allows for the most consistency when using various types of UDAs, especially when mixing functions and constructors: enum E { a, b, c }
enum myAttribute = E.a;
E myAttribute() { return E.a; }
struct MyAttribute {} // :( As for overloading, I see no reason why LuaD should not do automatic overload resolution. Manual overload resolution is not a practical solution when the code to marshall is in third party code, or standard library code. I also think the type checking needed will be extremely fast; checking Lua types is fast, and so is comparing Lua strings (required when a function is overloaded on userdata types). When overload sets are simple, overload resolution code will also be simple, and even for complex overload sets, I think branch prediction will help greatly. |
The point of @ScriptAffix is for use across a scope:
All of them will be prefixed appropriately, where rename wouldn't work. |
Also, I feel 'Script' in the attribute name is pretty superfluous. Why should it be there? |
It opens for generalization/cooperation later. Many programs support scripting in multiple languages. Having a common set of attributes like this between say, LuaD and PyD, seems beneficial to me.
I agree, but there are cases where it's not sufficient. For example, LuaD rejects functions with UTF16 or UTF32 parameter types. I don't know if this is the right solution for that particular problem, though. I guess the benefit is mostly for hiding certain functionality from scripts but not from D code without having to make a wrapper type just for the script-side, and for renaming symbols to match Lua conventions on the Lua side, while keeping the D style on the D side. edit: I realise now you're referring to the naming of the attributes. I was talking about the functionality of the attributes themselves versus say, using |
I see no reason why the binding to lua or python would be identical. Scripting languages are so wildly different, why would you expect such a clean mapping that a generic attribution would work? Is there some evidence to draw from?
Indeed, I'm all good with @noscript, that one's a no-brainer. I think @rename (and friends) may be useful, even in the presence of proper overload handling. I was referring to the need to write 'Script' in the attribute name. It's not like we're worried about name collisions. It's just cruft. I'd say make them one word attributes (except @noscript, which already has precedent in @nogc). |
- Shortened script UDAs and made them lowercase. - Split ScriptAffix into prefix and suffix. - Moved UDA logic to separate functions. - Cleaned up pushOverloadedMethod.
I think it should be pretty obvious. Pushing entire types and modules automatically makes sense for both Lua and Python, and in both cases it's useful to be able to hide certain symbols from the script.
I just think they're too specific; how about adding a replacement scheme to @rename("MyLib_$"):
int foo();
int bar(); Ideally it would also support capitalization so |
Adds support for the following attributes on class methods:
A::foo @ScriptRename(bar)
it'd be registered asA::bar
).Utility
Issues
This still doesn't solve the base problem of registering overloaded functions and ideally pushMethod and pushOverloadedMethod would be written with a more generic pushMethod as their base.
Example