Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Nullability annotations for System.Console #41161

Merged
merged 12 commits into from
Sep 19, 2019

Conversation

maryamariyan
Copy link
Member

@maryamariyan maryamariyan commented Sep 17, 2019

Contributes to: #40623

  • Annotate files included on unix configuration for System.Console

cc: @safern @buyaa-n

Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

LGTM except for the params comment I made which I'm pretty sure I'm remembering correctly but others might remember something else.

@@ -81,12 +82,12 @@ public static Encoding OutputEncoding
// s_out reinitialize the console code page.
if (Volatile.Read(ref s_out) != null && !s_isOutTextWriterRedirected)
{
s_out.Flush();
s_out!.Flush();
Copy link
Member

Choose a reason for hiding this comment

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

This ! is correct, but a little unfortunate. @jcouv, our use of [NotNullIfNotNull] on Volatile.Read was intended to cover cases like this, but it falls short because it's only one directional: the compiler can know that the return value is not null if s_out is not null, but it can't then also infer that s_out is not null if the return value is not null.

Copy link

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

@maryamariyan maryamariyan merged commit 19b304f into dotnet:master Sep 19, 2019
@maryamariyan maryamariyan deleted the sys-console-null branch September 19, 2019 02:50
@karelz karelz added this to the 5.0 milestone Dec 19, 2019
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Contributes to: dotnet/corefx#40623


Commit migrated from dotnet/corefx@19b304f
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants