Add Math.Clamp methods

[hughbe]

Open Questions

• Argument validation: should we check and enforce that max > min? Would this have a negative perf impact (that the old MaxMin approach didn't have)
• Double and Single: currently, any NaN argument means that NaN is returned - the clamp implementation in VC++ is undefined for NaN btw
• MathMin: I decided to implement Clamp without Math.Max(Math.Min(value, max), min) as I was unsure whether the JIT would inline these method calls (plus the code reads more easily IMO)
• Have I exposed the new APIs in the right place(model.xml)?

dotnet/corefx#467

/cc @sgtfrankieboy @weshaggard @mellinoe @mikedn

[sgtfrankieboy]

These lines should have 3 parameters instead of 2

[hughbe]

knew I was doing something wrong! Thansk

[mikedn]

Argument validation: should we check and enforce that max > min? Would this have a negative perf impact (that the old MaxMin approach didn't have)

With the current JIT adding an additional branch for the check will prevent the method from being inlined. For better or worse this can be avoided by using [MethodImpl(MethodImplOptions.AggressiveInlining)].

It's worth noting here that if the method is inlined and if the min/max arguments are constants (that's pretty common) then the check will be eliminated.

Double and Single: currently, any NaN argument means that NaN is returned - the clamp implementation in VC++ is undefined for NaN btw

It may make sense to return NaN if any argument is NaN but currently double.IsNaN generates poor code. And if you're implementing Clamp using Math.Min/Max then it's even worse.

The results you get if don't add special handling for NaNs (directly or indirectly by using Math.Min/Max) are IMO reasonable. You get NaN if the input value is NaN and the min/max checks are basically ignored if min/max are NaN.

[hughbe]

@mikedn thanks for taking a look - I'll add the argument validation and the inlining attribute as you suggested, and will remove the Math.Max(Math.Min...) stuff in double and single.
I've also rebased the PR to get it up to date with master

[mikedn]

Using object will result in boxing at the callsite, that will make clamp code larger. You could probably make this generic so boxing moves from the caller to the callee.

[hughbe]

fixed, thanks

[danmoseley]

would it potentially help debugging if the value appeared in the exception message also?

[hughbe]

It does, unless I misunderstand. The message is "'{0}' cannot be greater than {1}."

[bendono]

Has any consideration been given for a generic Clamp<T> constrained to T : IComparable<T>? This would greatly simplify the existing code which is essentially the same method defined 11 times for various argument types. It would also allow other types to be Clamped as long as they implement IComparable.

[sgtfrankieboy]

@bendono Yes, it has been discussed in this issue: dotnet/corefx#467

[hughbe]

@jkotas PTAL, thanks

[jkotas]

LGTM. Thanks!

[hughbe]

I was just looking at another PR updating src/mscorlib/model.xml, and I realised that they also update src/mscorlib/ref/mscorlib.cs.
I only did the former - do I need to add these to mscorlib.cs? I'm not sure what the difference is between these two files!

[jkotas]

src/mscorlib/ref/mscorlib.cs is temporary glue that is needed by the current corefx build. The corefx build should be switched to reference System.Private.CoreLib directly, like it does for corert. I would expect that this happens before these APIs get exposed in the contracts, so I do not think that it is necessary to add them to src/mscorlib/ref/mscorlib.cs.

cc @weshaggard

[weshaggard]

We are enabling people to start exposing APIs as .NET Core specific (see dotnet/corefx#11886). So if that is going to happen before we make the switch to building everything against S.P.CoreLib then these will need to be added to mscorlib ref. In either case it doesn't hurt to include them.