[generator] Extend skipInvokerMethods support to interfaces.
#1202
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jpobst
force-pushed
the
skip-interface-invokers
branch
from
03f8edf to
3b5f684
Compare
jonpryor
reviewed
Mar 6, 2024
| <class abstract='false' deprecated='not deprecated' final='false' name='Object' static='false' visibility='public' jni-signature='Ljava/lang/Object;' /> | ||
| </package> | ||
| <package name='com.xamarin.android' jni-name='com/xamarin/android'> | ||
| <interface abstract='false' deprecated='not deprecated' extends='java.lang.Object' extends-generic-aware='java.lang.Object' jni-extends='Ljava/lang/Object;' final='false' name='MyInterface' static='false' visibility='public' jni-signature='Lcom/xamarin/android/MyInterface;' skipInvokerMethods='com/xamarin/android/MyInterface.countAffectedRows()I'> |
There was a problem hiding this comment.
Generally (always?) //interface/@abstract will be true, not false. (At least, I can find no instances of such a thing in api-34.xml.)
jonpryor
reviewed
Mar 6, 2024
| @@ -331,6 +331,10 @@ public static InterfaceGen CreateInterface (XElement pkg, XElement elem, CodeGen | |||
| !options.SupportNestedInterfaceTypes | |||
| }; | |||
|
|
|||
| if (elem.Attribute ("skipInvokerMethods")?.Value is string skip) | |||
There was a problem hiding this comment.
This should be split out into a helper method, and CreateClass() updated to use the helper method, as I can't imagine any reason why the semantics for skipInvokerMethods should differ between classes and interfaces.
jonpryor
reviewed
Mar 6, 2024
| { | ||
| foreach (var m in methods) { | ||
| if (skipInvokers.Contains ($"{m.DeclaringType.RawJniName}.{m.JavaName}{m.JniSignature}")) |
There was a problem hiding this comment.
"Ditto" here: I'd be happier if ClassInvokerClass and InterfaceInvokerClass used the same method to generate the $"m.DeclaringType.RawJniName}.{m.JavaName}{m.JniSignature}" string.
jonpryor
reviewed
Mar 6, 2024
| @@ -331,6 +331,10 @@ public static InterfaceGen CreateInterface (XElement pkg, XElement elem, CodeGen | |||
| !options.SupportNestedInterfaceTypes | |||
| }; | |||
|
|
|||
| if (elem.Attribute ("skipInvokerMethods")?.Value is string skip) | |||
| foreach (var m in skip.Split (new char [] { ',', ' ', '\n', '\r' }, StringSplitOptions.RemoveEmptyEntries)) | |||
| iface.SkippedInvokerMethods.Add (m); | |||
There was a problem hiding this comment.
It occurs to me: should we "sanity check" this value, asserting that it contains one ., one (, and one )? (And maybe even verify that (…)… constitutes a valid JNI method signature, according to e.g. JniMemberSignature.GetParameterCountFromMethodSignature() + a return type sanity check.)
This request might be a step "too far" for this PR -- as opposed to my other comments, which I think are closer to "code hygiene" and code deduplication -- but this thought hadn't occurred to me before.
jpobst
force-pushed
the
skip-interface-invokers
branch
from
3b5f684 to
595625f
Compare
jonpryor
pushed a commit
that referenced
this pull request
Mar 6, 2024
Context: 73ebad2 Context: dotnet/android-libraries#779 Commit 73ebad2 added support for the metadata attribute `//class[@skipInvokerMethods]`, which allowed us to suppress generation of invoker methods for a class. The AndroidX Media3 binding in dotnet/android-libraries#779 hits a similar issue wherein we generate incorrect generics on an invoker type. However, this is an invoker type for an *`interface`*, not a `class`. Extend our `skipInvokerMethods` support so that it can be used as `//interface[@skipInvokerMethods]` as well.
jonpryor
added a commit
to dotnet/android
that referenced
this pull request
Mar 6, 2024
Context: dotnet/java-interop#1203 Does It Build™? Changes: dotnet/java-interop@bec0326...e557044 * dotnet/java-interop@e557044f: [generator] Extend `skipInvokerMethods` support to interfaces. (dotnet/java-interop#1202)
jonpryor
pushed a commit
to dotnet/android
that referenced
this pull request
Mar 7, 2024
Changes: dotnet/java-interop@14a9470...a7e09b7 * dotnet/java-interop@a7e09b7b: [generator] Extend `skipInvokerMethods` support to interfaces. (dotnet/java-interop#1202) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
jonpryor
added a commit
that referenced
this pull request
Mar 7, 2024
#1203) Context: 73ebad2 Context: dotnet/android-libraries#779 Commit 73ebad2 added support for the metadata attribute `//class[@skipInvokerMethods]`, which allowed us to suppress generation of invoker methods for a class. The AndroidX Media3 binding in dotnet/android-libraries#779 hits a similar issue wherein we generate incorrect generics on an invoker type. However, this is an invoker type for an *`interface`*, not a `class`. Extend our `skipInvokerMethods` support so that it can be used as `//interface[@skipInvokerMethods]` as well. Co-authored-by: Jonathan Pobst <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: #1086
#1086 added the metadata attribute
skipInvokerMethodsto allow us to suppress generation of invoker methods for a class.The AndroidX Media3 binding hits an issue where we generate incorrect generics in an invoker type. However this is an invoker type for an
interfaceinstead of aclass.Extend our
skipInvokerMethodssupport to cover interfaces as well.