Add Single and Double overloads to BinaryPrimitives by eerhardt · Pull Request #6864 · dotnet/runtime

eerhardt · 2020-01-31T00:39:13Z

GrabYourPitchforks · 2020-01-31T02:17:47Z

Thanks for catching these. :)

Unfortunately, it doesn't look like the docs are generated from this source. The stuff at https://docs.microsoft.com/en-us/dotnet/api/system.buffers.binary.binaryprimitives.trywriteint16bigendian is much better than what we have here.

The docs commented started with the comments from source but then went through doc review and got fixed up (see dotnet/dotnet-api-docs#2693). For new APIs, if you fix them in source, it will make porting them easier.

cc @carlossanlop

Why don't the fixes from the doc review get ported back into the source?

We do not have a automated tooling to do that. If we did have one, it would likely require a lot of manual interventions and be expensive to keep running on ongoing basis.

IMHO, the best way to address this would be to make the docs live in just one place, and avoid the duplication.

the best way to address this would be to make the docs live in just one place, and avoid the duplication.

Totally agree. IMO that one place should be in the source.

that one place should be in the source.

It is not obvious that it would be the right choice. The documentation is version-agnostic (spans all released and supported versions of .NET Framework and .NET Core), but the sources are version specific.

We had similar discussions previously with the docs team and given the constraints, decided as having the docs site be the single source of truth. It does have the side effect that you are seeing @eerhardt, but as @jkotas mentioned, trying to keep source and docs in sync is challenging, potentially flaky and may not be a tenable, especially since both the docs and source get updated all the time (from various parties). Given the real customer value is having the docs and Intellisense, we prioritized that as the source of truth and only "seed" them with the xml comments from source once. The higher order bit is to encourage and ensure that we are writing useful, customer facing xml comments that are complete, whenever we add new APIs and make sure the customer benefits from it. Area owners and those adding API/source contribution can manually sync them back if they choose to, but it isn't mandatory.

jkotas · 2020-01-31T03:19:38Z

@marek-safar Is it important to have endianess agnostic CoreLib for Mono?

If not, we can use ifdefs for all these instead that will make the code leaner and easier for the JIT to optimize.

ahsonkhan

Docs suggestions

eerhardt · 2020-01-31T20:29:26Z

Is it important to have endianess agnostic CoreLib for Mono?
If not, we can use ifdefs for all these instead that will make the code leaner and easier for the JIT to optimize.

My understanding was that JIT treats BitConverter.IsLittleEndian as a constant, and it eliminates unused code paths automatically. Is that good enough? Or do you think putting #if BIGENDIAN on all these methods is better?

See

runtime/src/libraries/System.Private.CoreLib/src/System/BitConverter.cs

Lines 21 to 27 in 47c2ba7

    
           #if BIGENDIAN 
        
                   [Intrinsic] 
        
                   public static readonly bool IsLittleEndian /* = false */; 
        
           #else 
        
                   [Intrinsic] 
        
                   public static readonly bool IsLittleEndian = true; 
        
           #endif

Fix dotnet#2365

eerhardt · 2020-01-31T23:11:01Z

I believe I have addressed the current feedback. PTAL and let me know if you have more feedback.

ahsonkhan

Some doc fix an -> a. Left suggestions to make it easier to fixup. Otherwise, LGTM!

marek-safar · 2020-02-03T13:36:14Z

Is it important to have endianess agnostic CoreLib for Mono?

It's not, CoreLib is runtime specific and we will distribute it with runtime anyway. However, libraries are so I am not sure if it's worth to have different code pattern used there especially if illinker is able to remove the condition

jkotas · 2020-02-05T06:12:48Z

if it's worth to have different code pattern used there especially if illinker is able to remove the condition

I have opened #31785 on this.

eerhardt requested review from GrabYourPitchforks, ahsonkhan and tannergooding January 31, 2020 00:39

GrabYourPitchforks reviewed Jan 31, 2020

View reviewed changes

jkotas requested changes Jan 31, 2020

View reviewed changes

Comment thread src/libraries/System.Private.CoreLib/src/System/Buffers/Binary/Reader.cs Outdated

stephentoub reviewed Jan 31, 2020

View reviewed changes

Comment thread src/libraries/System.Private.CoreLib/src/System/Buffers/Binary/ReaderBigEndian.cs Outdated

eerhardt requested a review from jkotas January 31, 2020 03:53

jkotas reviewed Jan 31, 2020

View reviewed changes

Comment thread src/libraries/System.Private.CoreLib/src/System/Buffers/Binary/ReaderLittleEndian.cs Outdated

jkotas approved these changes Jan 31, 2020

View reviewed changes

jkotas reviewed Jan 31, 2020

View reviewed changes

Comment thread src/libraries/System.Memory/tests/Binary/BinaryReaderUnitTests.cs Outdated

ahsonkhan reviewed Jan 31, 2020

View reviewed changes

ahsonkhan modified the milestones: 2.1.15, 5.0 Jan 31, 2020

ahsonkhan added the area-System.Runtime label Jan 31, 2020

GrabYourPitchforks mentioned this pull request Jan 31, 2020

Inefficient codegen for casts between same size types. #11413

Closed

eerhardt added 4 commits January 31, 2020 17:02

Add Single and Double overloads to BinaryPrimitives

fa9b7e3

Fix dotnet#2365

Address PR feedback

e7b5e1b

Add BinaryPrimitives Write tests for Single and Double.

96b203e

Respond to PR feedback

0d7bf1b

eerhardt force-pushed the Fix35791 branch from 997f4d3 to 0d7bf1b Compare January 31, 2020 23:09

ahsonkhan approved these changes Feb 1, 2020

View reviewed changes

Fix doc comment grammar.

d190b74

eerhardt merged commit e0a8bb4 into dotnet:master Feb 3, 2020

eerhardt deleted the Fix35791 branch February 3, 2020 23:48

maryamariyan added the new-api-needs-documentation label Feb 5, 2020

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Uh oh!

Conversation

eerhardt commented Jan 31, 2020

Uh oh!

Uh oh!

Uh oh!

GrabYourPitchforks Jan 31, 2020

Choose a reason for hiding this comment

Uh oh!

eerhardt Jan 31, 2020

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Jan 31, 2020

Choose a reason for hiding this comment

Uh oh!

jkotas Jan 31, 2020

Choose a reason for hiding this comment

Uh oh!

eerhardt Jan 31, 2020

Choose a reason for hiding this comment

Uh oh!

jkotas Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Jan 31, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jkotas commented Jan 31, 2020

Uh oh!

Uh oh!

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerhardt commented Jan 31, 2020

Uh oh!

eerhardt commented Jan 31, 2020

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marek-safar commented Feb 3, 2020

Uh oh!

jkotas commented Feb 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ahsonkhan Jan 31, 2020 •

edited

Loading

jkotas Jan 31, 2020 •

edited

Loading