Skip to content

Collect overridden Java methods in JCW #985

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

Merged
merged 1 commit into from
May 16, 2022

Conversation

grendello
Copy link
Contributor

Marshal methods generator in Xamarin.Android needs to know
which Java methods in which types are overridden by the application.

Package name, method name and signature are all require to generate
correct name of the native symbol which will be found by JNI at the
run time.

@grendello grendello requested review from jpobst and jonpryor May 16, 2022 11:29
@@ -80,6 +81,7 @@ public JavaCallableWrapperGenerator (TypeDefinition type, Action<string, object[
}
}

public List<string> OverriddenMethods => overriddenMethods;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be ICollection<string>, or maybe ReadOnlyList<string>. FxDG suggests avoiding List<T> for properties, as it's not easy to make changes later.

Naming: suggest OverriddenMethodDescriptions, as this is the names, not the methods themselves.

Instead of string, should a struct or class be used? I'm not sure how "future proof" a type-name:method-name string convention will work, and it might be useful in the future to store/retain type & method ids, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

FxDG ref: Guidelines for Collections:

❌ DO NOT use ArrayList or List in public APIs.

@jonpryor
Copy link
Contributor

Something appears to be "wrong" with the Mac - Mono bot, e.g. make all fails:

/Library/Frameworks/Mono.framework/Versions/6.12.0/lib/mono/msbuild/Current/bin/Microsoft.Common.CurrentVersion.targets(1232,5): error MSB3971: The reference assemblies for ".NETFramework,Version=v6.0" were not found. You might be using an older .NET SDK to target .NET 5.0 or higher. Update Visual Studio and/or your .NET SDK. [/Users/runner/work/1/s/tests/invocation-overhead/invocation-overhead.csproj]

This might be a sign we need to stop using msbuild?

@jpobst: thoughts?

Marshal methods generator in Xamarin.Android needs to know
which Java methods in which types are overridden by the application.

Package name, method name and signature are all require to generate
correct name of the native symbol which will be found by JNI at the
run time.
@jonpryor
Copy link
Contributor

WIP commit message:

[Java.Interop.Tools.JavaCallableWrappers] Collect overriden methods (#985)

We are prototyping a new marshal method generator in xamarin-android
which emits native functions with [JNI native method names][0].
This would remove the need for `Runtime.register()` invocations from
Java Callable Wrappers and all the related Reflection-heavy marshal
method registration code; see 4787e017 for some details for how the
current Reflection approach works.

In order to emit native functions which have the correct names, we
need to know additional information, such as the package name and
type name of the Java `native` method, method signature, etc.

Update `JavaCallableWrapperGenerator` to collect this additional
information, making it available via a new
`JavaCallableWrapperGenerator.OverriddenMethods` property.

[0]: https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#resolving_native_method_names

@jonpryor jonpryor merged commit fb94d59 into dotnet:main May 16, 2022
@grendello grendello deleted the marshal-methods branch May 16, 2022 13:44
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants