Skip to content

[Java.Interop] Fix C# warnings. #652

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 2 commits into from
Jun 1, 2020
Merged

[Java.Interop] Fix C# warnings. #652

merged 2 commits into from
Jun 1, 2020

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented May 29, 2020

  • Fixes 43 NRT warnings.
  • Fixes 1 CS0168 warning: (The variable 'e' is declared but never used)
  • Fixes 6 CA1307 warnings introduced by targeting netcoreapp3.1. (The behavior of 'string.IndexOf(char)' could vary based on the current user's locale settings.)

@jpobst jpobst changed the title [Java.Interop] Fix NRT warnings. [Java.Interop] Fix C# warnings. May 29, 2020
@jpobst jpobst marked this pull request as ready for review May 29, 2020 21:02
@@ -48,8 +48,8 @@ partial class CreationOptions {
var t = jie.GetType ("Java.Interop.MarshalMemberBuilder");
if (t == null)
throw new InvalidOperationException ("Could not find Java.Interop.MarshalMemberBuilder from Java.Interop.Export.dll!");
var b = (JniMarshalMemberBuilder) Activator.CreateInstance (t);
marshalMemberBuilder = SetRuntime (b);
var b = (JniMarshalMemberBuilder?) Activator.CreateInstance (t);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand this change, as Activator.CreateInstance() is only documented as returning null when type is a Nullable<T>, which won't be the case here.

Copy link
Member

Choose a reason for hiding this comment

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

I guess I prefer the approach taken later in this PR:

var b   = (JniMarshalMemberBuilder) Activator.CreateInstance (t)!;

@@ -87,7 +87,7 @@ public Delegate CreateMarshalToM
{
if (value == null)
throw new ArgumentNullException (nameof (value));
return CreateMarshalToManagedExpression (value.GetMethodInfo ()).Compile ();
return CreateMarshalToManagedExpression (value.GetMethodInfo ()!).Compile ();
Copy link
Member

Choose a reason for hiding this comment

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

.GetMethodInfo() can return null?!

Are the NRT annotations in mscorlib.dll "final" (or "sane")?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this was fixed in a later version, I'll take the change out and assume we will eventually be on the same version as dotnet: dotnet/runtime@505341d.

@@ -291,7 +290,7 @@ static unsafe JavaVMInterface CreateInvoker (IntPtr handle)
public virtual void FailFast (string? message)
{
var m = typeof (Environment).GetMethod ("FailFast");
m.Invoke (null, new object?[]{ message });
m?.Invoke (null, new object?[]{ message });
Copy link
Member

@jonpryor jonpryor May 30, 2020

Choose a reason for hiding this comment

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

This is a "FailFast" invocation. If m is null, for whatever reason, it should throw.

Or, we should add a Environment.Exit() or something on the following line.

The intent here is "the VM is hosed; we need to die"

@jpobst
Copy link
Contributor Author

jpobst commented Jun 1, 2020

Addressed feedback.

@jonpryor jonpryor merged commit 425f79d into master Jun 1, 2020
@jonpryor jonpryor deleted the ji-nrt branch June 1, 2020 19:37
@jpobst jpobst added this to the 10.5 (16.8 / 8.8) milestone Jun 5, 2020
jonpryor pushed a commit that referenced this pull request Jun 11, 2020
Fixes 43 Nullable Reference Type warnings

Fixes CS0168 warning:

	The variable 'e' is declared but never used.

Fixes 6 CA1307 warnings introduced by targeting `netcoreapp3.1`:

	The behavior of 'string.IndexOf(char)' could vary based on the current user's locale settings.
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 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