dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.27k stars 4.73k forks source link

Endianness comments in BitConverter.DoubleToInt32Bits and Int32BitsToDouble are irrelevant #4184

Closed sharwell closed 4 years ago

sharwell commented 9 years ago

Similar to the comments highlighted in dotnet/coreclr#833, the BitConverter.DoubleToInt32Bits and Int32BitsToDouble methods contain endianness comments which are almost certainly irrelevant.

I recommend these comments as well as the calls to Contract.Assert be removed, and the documentation updated to explicitly include the following text (or an equivalent variant) stating a precondition that has always existed but was previously undocumented.

This method assumes the hardward uses the same endianness for integers and floating-point numbers.

jkotas commented 9 years ago

I agree that this comment is bogus and should be removed.

mgravell commented 9 years ago

I'm happy to remove/revise those comments and assertions as part of that PR - no skin off my nose either way - but: this is my first coreclr PR: what is the usual process in the case of such a question / decision?

Minor clarification: it is DoubleToInt64Bits etc

sharwell commented 9 years ago

@mgravell I understand why you implemented your pull request the way you did, and believe it makes sense. This issue is meant to formalize the request to alter the original definitions so you can more comfortably remove them from yours as well. :smile: