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

[generator] Upgrade generator from monodroid/a438c473 #36

Merged
merged 5 commits into from
May 25, 2016

Conversation

jonpryor
Copy link
Member

Migrate multiple generator fixes from monodroid/a438c473.

Android N Preview 2 adds method parameters which are not valid C#
identifiers, and these weren't being properly handled before.

Fix parameter identifiers so that invalid characters such as `$` are
replaced with `_`.
Comments are inserted into the generated C# code to identify the
emitted member with the "source" XML declaration, to permit later
fixups via metadata without the drudgery of manually determining the
XPath references.

Unfortunately, when a class implements an interface, the generated
XPath comment for the interface method within the class would provide
an XPath expression "as if" the method were declared in the class,
which was not the case:

	// Metadata.xml XPath method reference: path="/api/package[@name='java.util']/class[@name='AbstractCollection']/method[@name='spliterator' and count(parameter)=0]"
	[Register ("spliterator", "()Ljava/util/Spliterator;", "GetSpliteratorHandler", ApiSince = 24)]
	public virtual unsafe Java.Util.ISpliterator Spliterator ()

Above, the spliterator() method is *actually* declared in
java.util.Spliterator<T>, but the comment implies that the generated
method is coming from java.util.AbstractCollection<T>. The comment is
thus wrong, as AbstractCollection.spliterator() doesn't exist within
the API XML (it's implied by the implemented interfaces).

To fix this, use the Method's DeclaringType in the XPath comment:

	// Metadata.xml XPath method reference: path="/api/package[@name='java.util']/class[@name='Spliterator']/method[@name='spliterator' and count(parameter)=0]"
In Android N Preview 2, `java.util.Iterator<E>` adds a `spliterator()`
method, which is overriden in `java.util.Collection<E>`.  That
resulted in our generator to emit duplicate binding methods for *both*
`spliterator()`s -- one from the `Iterator<T>`interface and one from
the `Collection<T>` interface -- within implementing classes such as
`java.util.AbstractCollection<T>`.

The most derived override should be the one to invoke, so generate
Java Callable Wrappers for only the most overridden ones
(`java.util.Collection<T>.spliterator()` in this case).
To do so, first mark them as overides, and then exclude those
overriden from the generated code.
Android N Preview 2 adds several static methods to the
java.util.Comparator<T> interface, such as `reverseOrder()`:

	/* partial */ interface Comparator<T> {
		public static <T extends java.lang.Comparable<? super T>>
			java.util.Comparator<T> reverseOrder();
	}

DO NOT BIND Java interface static methods within C# interfaces.

It doesn't make conceptual sense, and results in unusable bindings.
If we bind those methods into interfaces, our users will have to implement
those methods unlike Java8, and that's more annoying than missing them.

Related: dotnet#25
@atsushieno
Copy link
Contributor

You mentioned some different commit? " [generator] Fix build break."

@atsushieno
Copy link
Contributor

atsushieno commented May 25, 2016

merging as per our talk

@ jonp: was #36 to pick up missing cherry-picks or whatsoever missing only in that repo?
@ eno: yes, pick up functionality which was in monodroid but not OSS

@atsushieno atsushieno merged commit 86e0582 into dotnet:master May 25, 2016
radekdoulik added a commit to radekdoulik/java.interop that referenced this pull request Aug 27, 2018
Changes in xamarin-android-tools between 75530565b6aa903b3a0e52b61df4dd94475a19fc and 9e78d6ee586b498d0ea082b3bc00432c23583dd1:

9e78d6e (HEAD, origin/master, origin/HEAD, master) [tests] fix test failures on Windows (dotnet#47)
bdf0158 Better support no installed JDKs on macOS (dotnet#48)
6353659 Log what is happening during path selection (dotnet#46)
3ef860b Take BUILD_NUMBER into consideration for Version sorting (dotnet#45)
d3de054 Allow an optional locator to be provided to JdkInfo (dotnet#43)
917d3b3 Don't require quotes around `release` values (dotnet#41)
7427692 [tests] Unit tests for finding NDK location based on $PATH (dotnet#40)
dbc517b Merge pull request dotnet#38 from jonpryor/jonp-ndk-via-path
511d580 Allow finding NDK location based on $PATH
b42c217 [tests] Fix DetectAndSetPreferredJavaSdkPathToLatest() test (dotnet#37)
a4aad18 Add AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest() (dotnet#35)
fae7e0a [tests] Remove temporary directories (dotnet#36)
07c4c2b [Xamarin.Android.Tools.AndroidSdk] Revert JDK validation (dotnet#34)
radekdoulik added a commit that referenced this pull request Aug 27, 2018
Changes in xamarin-android-tools between 75530565b6aa903b3a0e52b61df4dd94475a19fc and 9e78d6ee586b498d0ea082b3bc00432c23583dd1:

9e78d6e (HEAD, origin/master, origin/HEAD, master) [tests] fix test failures on Windows (#47)
bdf0158 Better support no installed JDKs on macOS (#48)
6353659 Log what is happening during path selection (#46)
3ef860b Take BUILD_NUMBER into consideration for Version sorting (#45)
d3de054 Allow an optional locator to be provided to JdkInfo (#43)
917d3b3 Don't require quotes around `release` values (#41)
7427692 [tests] Unit tests for finding NDK location based on $PATH (#40)
dbc517b Merge pull request #38 from jonpryor/jonp-ndk-via-path
511d580 Allow finding NDK location based on $PATH
b42c217 [tests] Fix DetectAndSetPreferredJavaSdkPathToLatest() test (#37)
a4aad18 Add AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest() (#35)
fae7e0a [tests] Remove temporary directories (#36)
07c4c2b [Xamarin.Android.Tools.AndroidSdk] Revert JDK validation (#34)
@github-actions github-actions bot locked and limited conversation to collaborators Apr 15, 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.

3 participants