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

Implicit operator for Color to to enable int assignment #4120

Merged
merged 1 commit into from
Jan 16, 2020
Merged

Implicit operator for Color to to enable int assignment #4120

merged 1 commit into from
Jan 16, 2020

Conversation

mister-giga
Copy link
Contributor

it enables:
Color c = -65536; //Red
Simple conversion and intuitive since Color can be assigned to int parameter/field, vice versa shall be available.

it enables:
Color c = -65536; //Red
Simple conversion and intuitive since Color can be assigned to int parameter/field, vice versa shall be available.
@jonpryor
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonpryor jonpryor merged commit 1d5ed48 into dotnet:master Jan 16, 2020
@jonpryor
Copy link
Member

jonpryor commented Feb 24, 2020

There is a problem with this PR. Consider this code which calls SetScrimColor(int), in which .ToAndroid() returns an Android.Graphics.Color:

SetScrimColor(isShowingSplit ? Color.Transparent.ToAndroid() : (int)DefaultScrimColor);

This code compiles Just Fine with d16-5, but with this change it results in a CS0172 error:

Type of conditional expression cannot be determined because 'Color' and 'int' implicitly convert to one another.

Oops.

I fear we'll need to revert, as otherwise this would be an source compatibility change.

jonpryor pushed a commit that referenced this pull request Feb 25, 2020
…4315)

There was an unforeseen complication in commit 1d5ed48:
circular conversions.  Before 1d5ed48, `Android.Graphics.Color` had
an implicit conversion from `Color` to `int`.  Commit 1d5ed48 added
an implicit conversion from `int` to `Color`.  Thus:

	partial struct Color {
	    public static implicit operator Color (int value);
	    public static implicit operator int (Color value);
	}

This *seems* fine, but is problematic within ternary expressions:

	Color c1 = ...
	int   c2 = ...;
	var   c  = condition ? c1 : c2;

This results in a [CS0172 error][0]:

	error CS0172: Type of conditional expression cannot be determined because 'Color' and 'int' implicitly convert to one another	

In short, adding an implicit conversion from `int` to `Color` results
in a source compatibility break.

[Xamarin.Forms ran into this breakage][1]:

	SetScrimColor(isShowingSplit ? Color.Transparent.ToAndroid() : (int)DefaultScrimColor);

There are two potential fixes:

 1. Make the implicit `int`-to-`Color` operator *`explicit`*, or
 2. Remove the added operator entirely.

At this point in time, we see little benefit to making the operator
`explicit`, so we have opted to remove the operator by reverting
commit 1d5ed48.

[0]: https://docs.microsoft.com/en-us/dotnet/csharp/misc/cs0172
[1]: https://github.com/xamarin/Xamarin.Forms/blob/7a52542d50797ccc69ae1d8dd84259190d96bdb4/Xamarin.Forms.Platform.Android/Renderers/MasterDetailRenderer.cs#L425
jonpryor pushed a commit that referenced this pull request Feb 25, 2020
…4315)

There was an unforeseen complication in commit 1d5ed48:
circular conversions.  Before 1d5ed48, `Android.Graphics.Color` had
an implicit conversion from `Color` to `int`.  Commit 1d5ed48 added
an implicit conversion from `int` to `Color`.  Thus:

	partial struct Color {
	    public static implicit operator Color (int value);
	    public static implicit operator int (Color value);
	}

This *seems* fine, but is problematic within ternary expressions:

	Color c1 = ...
	int   c2 = ...;
	var   c  = condition ? c1 : c2;

This results in a [CS0172 error][0]:

	error CS0172: Type of conditional expression cannot be determined because 'Color' and 'int' implicitly convert to one another	

In short, adding an implicit conversion from `int` to `Color` results
in a source compatibility break.

[Xamarin.Forms ran into this breakage][1]:

	SetScrimColor(isShowingSplit ? Color.Transparent.ToAndroid() : (int)DefaultScrimColor);

There are two potential fixes:

 1. Make the implicit `int`-to-`Color` operator *`explicit`*, or
 2. Remove the added operator entirely.

At this point in time, we see little benefit to making the operator
`explicit`, so we have opted to remove the operator by reverting
commit 1d5ed48.

[0]: https://docs.microsoft.com/en-us/dotnet/csharp/misc/cs0172
[1]: https://github.com/xamarin/Xamarin.Forms/blob/7a52542d50797ccc69ae1d8dd84259190d96bdb4/Xamarin.Forms.Platform.Android/Renderers/MasterDetailRenderer.cs#L425
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 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.

4 participants