Issue #832; add BitConverter.SingleToInt32Bits and Int32BitsToSingle

[mgravell]

See Issue #832

[sharwell]

❓ Why include this comment? It's unlikely that the .NET Framework will ever support a system which uses different endianness for integers and floating point values (no one makes such a system anymore). In all other cases, this method behave as expected on both little- and big-endian machines.

[mgravell]

If I was writing it from scratch: I wouldn't have included it. I included
it because it was exactly as relevant or irrelevant as the corresponding
comment in the 64-bit method. I was going for consistency.
On 27 Apr 2015 2:44 pm, "Sam Harwell" notifications@github.com wrote:

In src/mscorlib/src/System/BitConverter.cs
#833 (comment):

•    [SecuritySafeCritical]
  

•    public static unsafe int SingleToInt32Bits(float value)
  

•    {
  

•        // If we're on a big endian machine, what should this method do?  You could argue for
  

•        // either big endian or little endian, depending on whether you are writing to a file that
  

•        // should be used by other programs on that processor, or for compatibility across multiple
  

•        // formats.  Because this is ambiguous, we're excluding this from the Portable Library & Win8 Profile.
  

•        // If we ever run on big endian machines, produce two versions where endianness is specified.
  

•        Contract.Assert(IsLittleEndian, "This method is implemented assuming little endian with an ambiguous spec.");
  

•        return _((int_)&value);
  

•    }
  

•    [SecuritySafeCritical]
  

•    public static unsafe float Int32BitsToSingle(int value)
  

•    {
  

•        // If we're on a big endian machine, what should this method do?  You could argue for
  

[image: ❓] Why include this comment? It's unlikely that the .NET
Framework will ever support a system which uses different endianness for
integers and floating point values (no one makes such a system anymore). In
all other cases, this method behave as expected on both little- and
big-endian machines.

—
Reply to this email directly or view it on GitHub
https://github.com/dotnet/coreclr/pull/833/files#r29146605.

[ghost]

👍 for consistency!

[jkotas]

This Assert/comment is bogus. It would be better to not add it here; and delete it from the two other methods instead.

[jkotas]

cc @AlexGhiondea @weshaggard Could you please route this as appropriate?

[weshaggard]

@mgravell thanks for the interest and contribution but I have to ask you to please follow the API review process for new public additions.

[mgravell]

@weshaggard issue was opened; haven't had much input to date. If I've jumped ahead too fast with providing code: that is because there is exactly one way to implement this that is consistent with the preexisting 64-bit API. What would you me do/done differently? I have extended the issue a bit; is this your main concern?

[weshaggard]

@mgravell thanks for adding the extra detail in the issue we can use that to track the review and simply keep the PR open given you have already put it together. I will tag @terrajobst, who drives our reviews to have a look and get it included in our next review session.

What you are proposing seems very reasonable to me but we are very cautious about adding new public APIs into our core library which is why we have put the API Process in place. Generally we want to get approval of the API's before actually submitting a PR.

[jskeet]

Just as a note of support, I've wanted exactly this sort of thing multiple times. It's always seemed an oddly-missing piece of functionality.

[richlander]

FYI: @terrajobst is out right now. So, we're probably not going to make progress on this one until later in the month.

[ghost]

@terrajobst, is this still under consideration? :)

[drewnoakes]

This API extension is also mentioned in 1151.

[ghost]

@drewnoakes, you mean https://github.com/dotnet/corefx/issues/1151? 😉

[drewnoakes]

@jasonwilliams200OK yes, thanks for spotting that and apologies for the mistake.

[NickCraver]

Is there any update on the review of this? The review on https://github.com/dotnet/corefx/issues/1151 involves a much wider set of additions and more questions - where this is much more straightforward.

[AlexGhiondea]

@NickCraver thanks for the code. However, we still need to get this API reviewed following the API Review process.

@terrajobst could we get this and dotnet/corefx#1151 reviewed?

[weshaggard]

@AlexGhiondea this and the associated issue aren't currently following the API review proces so it will not show up on fxdc's radar.

[ghost]

@weshaggard, this issue was opened before the API review process came into existence. Should it be treated as 'legacy' in that regard?

[weshaggard]

@jasonwilliams200OK it still needs to go through the process to get on the radar as we have a lot of issues to review and when we go through the backlog it will not be looked at if it isn't correctly tagged. The owner of these API area, which is @AlexGhiondea should be the one doing the first round of review and then if he thinks it ready he should tag it as ready for review.

[mgravell]

@weshaggard I have to bite my lip a bit here to avoid coming off flippant. Back in may, you asked me to follow a specific review process (now a dead link). I duly complied. Now, after months of nothing, the problem is that I'm not following a completely new, completely different review process that I've never heard of. I appreciate that you need a process for managing these things, but changing the guidelines without telling anyone, and without "grandfathering in" - is pretty crappy.

Sigh. When I summon up some energy, I'll see what hoops I need to jump through this time.

[weshaggard]

@mgravell I must apologize to you about how long this as taken and I fully understand your frustration. My comments about following the process is more directed at @AlexGhiondea who is the owner of this area, you have actually done everything I think you need as part of https://github.com/dotnet/coreclr/issues/832. We really need to get organized and work through our huge backlog, and so when I saw @AlexGhiondea simply ask for a review in the comments I wanted to make sure he followed the process to correctly tag the issues so that it shows up on the radar as that is the only sure way to make sure we see it.

[AlexGhiondea]

@mgravell, I would like to join @weshaggard in appologizing for how long this process has take thus far.

Our processes are always evolving based on our experience and the feedback we are receiving.
After being away from this space for a while, I am still getting used to the current process myself.

I looked at your proposal and it looks reasonable to me. I think the APIs would nicely complement the existing ones and I don't believe there is a compat concern here. The implementation is so straightforward that I think we should have added them from the beginning (for completness if nothing else).

The thing that I am considering is the work required to add these APIs to mscorlib. In our current state (when we are halfway between TFS and GitHub) adding an API to mscorlib is a somewhat involved process which requires changes to two repos and a bunch of changes in our internal TFS repo as well.

We are working on making this a better place where changes like this won't be so costly.

That being said, I do think we should bring this in to FXDC as these APIs would be a good addition to the existing ones.

[AlexGhiondea]

Hmm... It looks like the CoreCLR repo does not have the same tags as the CoreFX repo and so it won't show up on the FXDC's radar.

@mgravell - I can move the issue over to CoreFX and tag it appropriately there

[AlexGhiondea]

@mgravell I have migrated this over to the CoreFx repo here and marked it as ready to review.

[mgravell]

Much appreciated

On 25 November 2015 at 19:03, AlexGhiondea notifications@github.com wrote:

@mgravell https://github.com/mgravell I have migrated this over to the
CoreFx repo here https://github.com/dotnet/corefx/issues/4673 and
marked it as ready to review.

—
Reply to this email directly or view it on GitHub
#833 (comment).

Regards,

Marc

[Petermarcu]

@AlexGhiondea @weshaggard , I see this was approved from API review, lets try to get this one in!

[terrajobst]

@karelz who should this be assigned to?

[jkotas]

The new methods have to be added to model.xml. They will be stripped by CoreCLR BCL rewriter otherwise.

[karelz]

@jkotas I assume model.xml is specific to coreclr repo, right? Or does it apply also to corefx repo?
Do we have 'how to' docs for that?

@AlexGhiondea can you please help finish it?
cc: @danmosemsft

[jkotas]

@karelz The how-to doc is at https://github.com/dotnet/coreclr/blob/master/Documentation/project-docs/adding_new_public_apis.md

[AlexGhiondea]

@karelz the model.xml file is in the coreclr repo. The 2 new methods need to be added to the type inside model.xml.

[Petermarcu]

@mgravell did you want to finish this up. The model.xml file referenced is here: https://github.com/dotnet/coreclr/blob/master/src/mscorlib/model.xml

If not, @AlexGhiondea , can you finish this up?

[AlexGhiondea]

@dotnet-bot please test OSX x64 Checked Build and Test, Ubuntu x64 Checked Build and Test

[AlexGhiondea]

@mgravell you would need to add something like this:

<Member Name="SingleToInt32Bits(System.Single value)" />
<Member Name="Int32BitsToSingle(System.Int32 value)" />

In model.xml under the System.BitConverter section.

Let me know if you would rather I do it.

[jkotas]

This fix was included in #5492