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

Revert making Nullable<T> readonly #15198

Merged
merged 2 commits into from Nov 23, 2017

Conversation

Projects
None yet
3 participants
@stephentoub
Copy link
Member

stephentoub commented Nov 23, 2017

@stephentoub stephentoub force-pushed the stephentoub:nullable_revert branch from 23cddbc to 5547606 Nov 23, 2017

{
private readonly bool hasValue; // Do not rename (binary serialization)
internal readonly T value; // Do not rename (binary serialization)
internal T value; // Do not rename (binary serialization)

This comment has been minimized.

Copy link
@jkotas

jkotas Nov 23, 2017

Member

Add a comment why this cannot be readonly?

This comment has been minimized.

Copy link
@stephentoub

stephentoub Nov 23, 2017

Author Member

Will add. The test I've added in corefx will also fail if this is made readonly.

@stephentoub stephentoub merged commit f2fbb1c into dotnet:master Nov 23, 2017

9 of 15 checks passed

CentOS7.1 x64 Checked Innerloop Build and Test Started.
Details
OSX10.12 x64 Checked Innerloop Build and Test Started.
Details
Ubuntu armlb Cross Debug Innerloop Build Started.
Details
Ubuntu x64 Checked Innerloop Build and Test Started.
Details
Ubuntu16.04 armlb Cross Debug Innerloop Build Started.
Details
Windows_NT x64 Checked Innerloop Build and Test Started.
Details
CROSS Check Build finished.
Details
CentOS7.1 x64 Debug Innerloop Build Build finished.
Details
Tizen armel Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm64 Cross Debug Innerloop Build Build finished.
Details
Ubuntu x64 Innerloop Formatting Build finished.
Details
WIP ready for review
Details
Windows_NT x64 Innerloop Formatting Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test Build finished.
Details
license/cla All CLA requirements met.

@stephentoub stephentoub deleted the stephentoub:nullable_revert branch Nov 23, 2017

dotnet-bot added a commit to dotnet/corert that referenced this pull request Nov 23, 2017

Revert making Nullable<T> readonly (dotnet/coreclr#15198)
* Revert making Nullable<T> readonly

* Address PR feedback

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

stephentoub added a commit to dotnet/corert that referenced this pull request Nov 23, 2017

Revert making Nullable<T> readonly (dotnet/coreclr#15198)
* Revert making Nullable<T> readonly

* Address PR feedback

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
@VSadov

This comment has been minimized.

Copy link
Member

VSadov commented Nov 23, 2017

After thinking a bit about this, it looks like a bug/oversight in Nullable.
ToString should do something like ‘this?.ToString() ?? “” ‘

Mutable Nullable is less convenient and is unlikely intentional. Methods like ToString can change the state. Even T being readonly or a primitive would not help much. ToString could still do anything - as gross as ‘this = null’.

I guess we are keeping it as is anyways...

dotnet-bot added a commit to dotnet/corefx that referenced this pull request Jan 13, 2018

Revert making Nullable<T> readonly (dotnet/coreclr#15198)
* Revert making Nullable<T> readonly

* Address PR feedback

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

dotnet-bot added a commit to dotnet/corefx that referenced this pull request Jan 13, 2018

Revert making Nullable<T> readonly (dotnet/coreclr#15198)
* Revert making Nullable<T> readonly

* Address PR feedback

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

safern added a commit to dotnet/corefx that referenced this pull request Jan 16, 2018

Revert making Nullable<T> readonly (dotnet/coreclr#15198)
* Revert making Nullable<T> readonly

* Address PR feedback

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

safern added a commit to dotnet/corefx that referenced this pull request Jan 16, 2018

Revert making Nullable<T> readonly (dotnet/coreclr#15198)
* Revert making Nullable<T> readonly

* Address PR feedback

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.