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

New PhysicalAddress.TryParse methods taking span and string #1057

Merged

Conversation

alnikola
Copy link
Contributor

This PR introduces two new PhysicalAddress.TryParse methods taking span and string as well as adds an PhysicalAddress.Parse overload taking span.
Fixes dotnet/corefx#29780

New PhysicalAddress.Parse overload taking span
@alnikola
Copy link
Contributor Author

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@scalablecory scalablecory left a comment

Choose a reason for hiding this comment

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

Looks good other than this one thing.

@alnikola
Copy link
Contributor Author

alnikola commented Jan 7, 2020

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@alnikola alnikola merged commit 66cb5f9 into dotnet:master Jan 7, 2020
@alnikola alnikola deleted the alnikola/29780-ro-span-physical-address branch January 7, 2020 13:40
@@ -103,22 +103,39 @@ public byte[] GetAddressBytes()
return (byte[])_address.Clone();
}

public static PhysicalAddress Parse(string address)
public static PhysicalAddress Parse(string address) => address != null ? Parse(address.AsSpan()) : None;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems before the PR Parse(null) throw.

For string.Empty we throw too so we could use implicit conversion string->ReadOnlySpan

public static PhysicalAddress Parse(string address) => Parse(address);

Copy link
Member

@stephentoub stephentoub Jan 7, 2020

Choose a reason for hiding this comment

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

It seems before the PR Parse(null) throw.

No, it returned None:
https://github.com/dotnet/runtime/pull/1057/files#diff-74838e07dff5967e2e1173264255be64L112-L115

Copy link
Contributor

Choose a reason for hiding this comment

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

I get the exception in PowerShell console (.Net Core 3.1)
image

Copy link
Member

@stephentoub stephentoub Jan 7, 2020

Choose a reason for hiding this comment

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

I don't know what PowerShell is passing there, but passing in null returns None.

using System;
using System.Net.NetworkInformation;

class Program
{
    static void Main()
    {
        Console.WriteLine(PhysicalAddress.Parse(null) == PhysicalAddress.None);
    }
}

outputs:

True

That's the case for .NET Framework as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@stephentoub Thanks! I will open new issue in PowerShell repo.

@stephentoub
Copy link
Member

@alnikola, you merged this with a bunch of failures due to this PR that are causing stack overflow exceptions.

@alnikola
Copy link
Contributor Author

alnikola commented Jan 8, 2020

Sorry for the mistake I was confused by the CI run results. It reported 73 successful checks out of 83 which made me to believe that the code is fine since I didn't do anything platform specific.

@stephentoub
Copy link
Member

The build legs and the test legs are separate. Lots of build legs succeeded, but the legs that actually ran the tests failed.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
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.

6 participants