Remove extra well-known members from Range#32805
Conversation
There was a problem hiding this comment.
Perhaps factor this block out: left = optimizeOrCreateIndex(left, fromEnd: false);? Possibly could be factored into a single local function (ie. merge the logic with tryOptimizeOperand).
There was a problem hiding this comment.
""System.Range System.Range.FromStart(System.Index)"" [](start = 23, length = 53)
Is there a design/speclet document that should be updated to reflect the compiler/API design?
It may be good to send an email to LDM (and relevant BCL folks) to let them know of this change and rationale.
|
Updated PR to use the new API from CoreFX. The well-known members are described in dotnet/csharplang#2214, although this PR doesn't use any of the optional members. |
|
Note: you'll want to re-target to |
|
@jcouv Good point, thanks. |
We previously required more members than strictly necessary for range syntax. This change makes it so we only require one member. It may be useful for performance to use those members in the future, but right now optimal range performance is not what we're focusing on.
9e3348a to
a1998ab
Compare
| @@ -121,12 +134,11 @@ private BoundExpression LiftRangeExpression(BoundRangeExpression node, Immutable | |||
| Debug.Assert(condition != null); | |||
|
|
|||
| // method(left.GetValueOrDefault(), right.GetValueOrDefault()) | |||
There was a problem hiding this comment.
method [](start = 15, length = 6)
nit: comment seems out-of-date.
Also, name of the local (rangeCall) should probably be rangeCreation
| { | ||
| public readonly struct Index | ||
| using System.Runtime.CompilerServices; | ||
| public readonly struct Index : IEquatable<Index> |
There was a problem hiding this comment.
Index [](start = 27, length = 5)
I assume this is copied from corefx, so doesn't need reviewing. Let me know if otherwise.
We currently require more members than strictly necessary for range
syntax. This change makes it so we only require one member. It may be
useful for performance to use extra members in the future, but right
now optimal range performance is not what we're focusing on and this
allows CoreFX to change the names without require a compiler change.