Skip to content

Conversation

@carlossanlop
Copy link
Contributor

No description provided.

@carlossanlop carlossanlop added new-content Indicates PRs that contain new articles waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews 🏁 Release: .NET Core 2.x Identifies work items for the .NET Core 2.x releases labels Sep 27, 2019
@carlossanlop carlossanlop added this to the September 2019 milestone Sep 27, 2019
@carlossanlop carlossanlop self-assigned this Sep 27, 2019
Copy link

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

I've left some suggestions and comments, @carlossanlop.

]]></format>
</remarks>
<exception cref="T:System.ArgumentNullException"><paramref name="input" /> is <see langword="null" />.</exception>

Choose a reason for hiding this comment

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

Suggested change
<exception cref="T:System.ArgumentNullException"><paramref name="input" /> is <see langword="null" />.</exception>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rpetrusha Thanks for catching this. I tested this:

static void Main(string[] args)
{
    ReadOnlySpan<char> input = null;
    Guid guid = Guid.Parse(input);
}

and we end up throwing System.FormatException: Unrecognized Guid format.

Looking at the code for Parse, this method does not check if input is null, and it directly passes the argument to the private method TryParseGuid, which also does not check if input is null.
But it seems that calling Trim on a null ReadOnlySpan<char> will actually be calling it on an empty span. Not sure how this works, but it seems the span never becomes null. I'll dig more into this later.

Choose a reason for hiding this comment

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

A span can't be null, @carlossanlop, since it's a value type.

@carlossanlop carlossanlop added changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review and removed waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews labels Oct 3, 2019
@carlossanlop carlossanlop requested a review from rpetrusha October 3, 2019 18:25
@rpetrusha
Copy link

Thanks, @carlossanlop. I'll merge this PR now.

@rpetrusha rpetrusha merged commit 4197154 into dotnet:master Oct 4, 2019
@carlossanlop carlossanlop deleted the Guid branch October 7, 2019 16:30
@mairaw mairaw removed the changes-addressed Indicates PRs that had all comments addressed and are awaiting for new review label Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏁 Release: .NET Core 2.x Identifies work items for the .NET Core 2.x releases new-content Indicates PRs that contain new articles

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants