Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Update BitConverter from CoreFX#9788

Merged
jkotas merged 2 commits into
dotnet:masterfrom
jkotas:BitConverter
Feb 26, 2017
Merged

Update BitConverter from CoreFX#9788
jkotas merged 2 commits into
dotnet:masterfrom
jkotas:BitConverter

Conversation

@jkotas
Copy link
Copy Markdown
Member

@jkotas jkotas commented Feb 25, 2017

Related to #9701

@mikedn
Copy link
Copy Markdown

mikedn commented Feb 25, 2017

Would it make sense to copy the initialization of IsLittleEndian from corefx too? We'd avoid an #if and when IsLittleEndian becomes an intrinsic the initialization cost won't be a concern anymore.

@jkotas
Copy link
Copy Markdown
Member Author

jkotas commented Feb 25, 2017

I kept it because of there are number of BIGENDIAN ifdefs in corelib today, and so this is following the corelib style for endianness specific code.

@jkotas jkotas added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 25, 2017
@jkotas
Copy link
Copy Markdown
Member Author

jkotas commented Feb 25, 2017

Wait for the current CoreCLR drop to stabilize in corefx before pushing this through (see dotnet/corefx#16454)

@jkotas
Copy link
Copy Markdown
Member Author

jkotas commented Feb 25, 2017

@stephentoub I see you have commented on the other PR related to this. Could you please take a look?

Comment thread src/mscorlib/corefx/SR.cs

internal static string ArgumentOutOfRange_GenericPositive
{
get { return Environment.GetResourceString("ArgumentOutOfRange_GenericPositive"); }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: Not for this PR, but at some point subsequently we might want to go through and replace all of this strings with nameof, e.g.

using static Environment;
...
internal static string ArgumentOutOfRange_GenericPositive => GetResourceString(nameof(ArgumentOutOfRange_GenericPositive);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The plan is switch this file to be auto-generated like everywhere else(https://github.com/dotnet/coreclr/issues/9474)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Even better.

Comment thread src/mscorlib/src/System/BitConverter.cs Outdated

private static void ThrowValueArgumentNull()
{
throw new ArgumentNullException("value");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did you purposefully decide against ThrowHelper for these exceptions?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not really ... just copied what was in corefx. I have fixed it up to use ThrowHelper.

@hqueue
Copy link
Copy Markdown
Member

hqueue commented Feb 25, 2017

@dotnet-bot test Linux ARM Emulator Cross Release Build
@dotnet-bot test Linux ARM Emulator Cross Debug Build

@jkotas jkotas removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 26, 2017
@jkotas jkotas merged commit a5e0190 into dotnet:master Feb 26, 2017
@jkotas jkotas deleted the BitConverter branch February 26, 2017 05:31
jorive pushed a commit to guhuro/coreclr that referenced this pull request May 4, 2017
* Update BitConverter from CoreFX

Related to #9701

* Use ThrowHelper
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Update BitConverter from CoreFX

Related to dotnet/coreclr#9701

* Use ThrowHelper

Commit migrated from dotnet/coreclr@a5e0190
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.

6 participants