UInt64 comparison operators ambiguity #2322
Replies: 29 comments
-
you can save one // if (n2 < 0) return 1; // no need to check. if n1 is smaller than 128 it can be safely cast to sbyte
if (n1 > 127ul ) return 1;
return ((sbyte)n1).CompareTo(n2); or this way if (n2 < 0) return 1;
// if (n1 > 127ul ) return 1; // no need to check. if n2 is bigger than 0 it can be safely cast to ulong
return (n1).CompareTo((ulong)n2); |
Beta Was this translation helpful? Give feedback.
-
@MkazemAkhgary
I don't know what has a better performance: Checking a value of a type, or casting a value to another type. If the cost of casting is low, then tour code is better, otherwise, we use as many Note: My issue here is not how to fin a workaround. I simply want the compiler to fix this issue so we can use the intrinsic type ulong as expected. |
Beta Was this translation helpful? Give feedback.
-
Relevant portion of the spec: https://github.com/dotnet/csharplang/blob/master/spec/types.md#integral-types
The spec doesn't allow for a resolution here as there is no way to convert both The simplest workaround seems to be a shorter version of the At the moment this can be solved with user-defined methods including extension methods. For now, there's nothing that CoreFX or Roslyn can do unless the LDM changes the spec. |
Beta Was this translation helpful? Give feedback.
-
@yaakov-h |
Beta Was this translation helpful? Give feedback.
-
Roslyn has to follow the spec. |
Beta Was this translation helpful? Give feedback.
-
@yaakov-h to be fair the spec explicitly forbids only |
Beta Was this translation helpful? Give feedback.
-
@yaakov-h @miloush ulong a = 0;
z = a > long.MaxValue; C# compiles the above code correctly. It casts long.MaxValue to ulong, because if is obviously safe to do so. but this is not the only safe case, as I showed in my proposal. C# compiler needs only to do a little more checks. |
Beta Was this translation helpful? Give feedback.
-
I removed my comment of I think casting is cheaper than |
Beta Was this translation helpful? Give feedback.
-
@VBAndCs unfortunately I think in that case it is following the spec, as I imagine both operands are of type |
Beta Was this translation helpful? Give feedback.
-
While we are in the subject of numeric casts, there is a problem in casting an object that contains a numeric type to another numeric type: object o = 1;
var a = (int)o; // OK
var c = (long)o; // runtime error (InvalidCastException)
var d = Convert.ToUInt64(o); //Ok
var d = (long)(int)o; //Ok
o = (byte)1;
var x = (int)o; // runtime error (InvalidCastException)
var y = Convert.ToUInt32(o); //Ok
var z = (int)(byte)o; //Ok Can't C# compiler do something about that? Why should I write two casts, while the Convert class does it directly? |
Beta Was this translation helpful? Give feedback.
-
@VBAndCs I suggest you open another issue on that topic. |
Beta Was this translation helpful? Give feedback.
-
I think casting involving creating a new type, and in your example you cast to the bigger-size type, which is relatively slower in comparison. |
Beta Was this translation helpful? Give feedback.
-
My point is that C# decided to make the cast from long to ulong in this specific case, which means that C# does some checks. any other long value will not be casted including long.MinValue. So, C# can add some more checks to resolve the comparison operation ambiguity. |
Beta Was this translation helpful? Give feedback.
-
On a 64-bit machine arithmetic operations are going to be 64-bit anyways. the casting doesn't create a new type, I think this is going to be performed on CPU registers so it would be insanely fast. branches are more expensive because of the cost of branch prediction. |
Beta Was this translation helpful? Give feedback.
-
At the same time, others might expect that types such as There are of course exceptions. For example But you'll have a hard time convincing language designers to support such mixed type comparisons. Especially when usage of unsigned type in C# is very low and when other similar languages (C/C++, Java) do not support this either. |
Beta Was this translation helpful? Give feedback.
-
As I said, this is only related to ulong. We can compare byte to int, sbyte to ushort (both casted to int), short to uint (bith casted to long). The expected behaviour with ulong and signed type is to cast both to decimal to do the comparison. If this is costy, then c# can workarrount to emit the nececaty IL code to do the comparison with the smallest size type as I showwd. |
Beta Was this translation helpful? Give feedback.
-
A code-fix might be useful here, and definitely has a much lower bar for acceptance. That would need to be discussed over on the Roslyn repo. |
Beta Was this translation helpful? Give feedback.
-
So this repo is for when you think changing the specification is the right thing to do. Your option basically would be to propose changing
into
That however includes operators you don't care about and I suspect might not go very well. The other option you have is to ask for this case to be explicitly mentioned as allowed in the specification. If you don't care about the specification and want the compiler to support this comparison, this should go to the Roslyn repo as @yaakov-h said. I haven't tried, but I am guessing that changing the appropriate ERR to DEC in this table would be the simplest change that would result in the behaviour you want. However, I imagine emitting the negative check you suggest would be preferred to decimal precision operation from the performance point of view - and could still keep Moreover, the decimal is not as 1st class citizen as the other integral types. For example,
Allowing "implicit decimal comparisons" without bringing the decimal experience on par with other types anywhere else sounds like a hack that would need some justification.
That also goes to the Roslyn repo. However, note that casting
I think this is the strongest point you have. The compiler error message is misleading and I would support the request to make the problem clearer or suggest alternatives. You could probably create a quick fix that does the work for you too.
My point was that this is not a runtime cast. The compiler treats it as an ulong a = 0;
const long m = long.MaxValue;
bool z = a > m; as you noticed works. It's ulong a = 0;
long m = long.MaxValue;
bool z = a > m; results in the error. |
Beta Was this translation helpful? Give feedback.
-
@miloush |
Beta Was this translation helpful? Give feedback.
-
@VBAndCs Don't worry, someone from the team will join soon and give some background on this/suggestions. |
Beta Was this translation helpful? Give feedback.
-
@miloush ulong a = 0;
bool z = a > sbyte.MaxValue; This code will not compile because C# sees it ambiguous! It has no problem with |
Beta Was this translation helpful? Give feedback.
-
@VBAndCs looks like a bug since MaxValue is constant. |
Beta Was this translation helpful? Give feedback.
-
Seems correct to me. https://github.com/dotnet/csharplang/blob/master/spec/expressions.md#binary-numeric-promotions says:
Now I suppose it's kind of strange that It does indeed seem that the error message could be improved. I don't really know what "ambiguous" means in static bool GreaterThan(int x, int y) => x > y;
static bool GreaterThan(long x, long y) => x > y;
static bool GreaterThan(uint x, uint y) => x > y;
static bool GreaterThan(ulong x, ulong y) => x > y;
static bool Test(ulong x) => GreaterThan(x, sbyte.MaxValue); the error message is |
Beta Was this translation helpful? Give feedback.
-
|
Beta Was this translation helpful? Give feedback.
-
That doesn't explain why sbyte constant cant be converted to ulong. I don't see reason why it shouldn't
As far as i know, constants are evaluated at compile time, its perfectly fine to implicitly convert |
Beta Was this translation helpful? Give feedback.
-
I think the problem is that sbyte, ushort, uint and ulong are rarely used, so they not tested well and there was no push to perfect their work and update the specs. |
Beta Was this translation helpful? Give feedback.
-
I think the problem is that sbyte, ushort, uint and ulong are rarely used, so they not tested well and there was no push to perfect their work and update the specs. |
Beta Was this translation helpful? Give feedback.
-
I think the spec is quite intentional about these types and their signed/unsigned counterparts. You not liking the behavior doesn't mean that it is poorly designed or haphazard. |
Beta Was this translation helpful? Give feedback.
-
Why? From the spec:
AFAIK, It shouldn't because the spec does not allow it, unless by not explicitly permitting something, the spec allows all kinds of implementation. ( |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
I posted this in https://github.com/dotnet/corefx/issues/35890 but it seems to be language related, so I posted it here too:
Using comparison operators (>, < , etc…) between UInt64 type and any signed type like sbyte , short, int and long causes ambiguity in C#. Try:
it will cause the error:
I understand that arithmetic operations between UInt64 type and any signed type is tricky because we can't predict the resultant type and the user must do the necessary casting according to his needs, but a comparison is a deterministic operation that yields a Boolean value, so there is no way to be ambiguous. The easiest workaround is to cast the ulong to decimal to do the comparison between two decimals, which comes with a high cost and seems unnecessary!
I wrote these functions to compare ulong and signed types. I suggest you use the same approach to define necessary overloads for the CompareTo method and operator overloads in UInt64, SByte, Int16, Int32 and Int64 , so comparisons between ulong and signed types becomes unambiguous.
Note:
All positive types other than ulong can be compared normally to any signed types. That is becuase the language uses the comparison operator of int or long. The problem of ulong is that it needs to be casted to decimal and this is not effiecint, so the language markd it as ambiguity, while there is a rather cheap, logiacl and easy slution for this.
Beta Was this translation helpful? Give feedback.
All reactions