Add Single and Double overloads to BinaryPrimitives

[eerhardt]

Fix #2365

[GrabYourPitchforks]

Thanks for catching these. :)

[eerhardt]

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.

[ahsonkhan]

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

[eerhardt]

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

[jkotas]

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.

[eerhardt]

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.

[jkotas]

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.

[ahsonkhan]

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]

@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]

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

[eerhardt]

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]

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]

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.