Constant folding for native integer values #42143
Conversation
|
|
||
| constantFolding("nint", "const nint A = -2147483648;", "~A", "2147483647"); | ||
| constantFolding("nint", "const nint A = 2147483647;", "~A", "-2147483648"); | ||
| constantFolding("nuint", "const nuint A = 0;", "~A", "4294967295"); // PROTOTYPE: Should this be an error? |
There was a problem hiding this comment.
I don't think these are correct. These assume a nint is 32 bits, which is only correct on some platforms. Since we don't know how many bits there are in a nint, we cannot fold the ~ operation. #Resolved
| Array.Empty<DiagnosticDescription>() : | ||
| new[] { diagnostic }; | ||
| verify(opType, declarations, expr, expectedResult, diagnostics); | ||
| //verify(opType, declarations, $"checked ({expr})", expectedResult, diagnostics); |
There was a problem hiding this comment.
// [](start = 16, length = 2)
Not sure why you commented this line. #Resolved
There was a problem hiding this comment.
|
Done with review pass (iteration 4), tests are not looked at. #Closed |
|
|
||
| void constantFolding(string opType, string declarations, string expr, string expectedResult, DiagnosticDescription diagnostic = null) | ||
| { | ||
| var diagnostics = (diagnostic is null) ? |
There was a problem hiding this comment.
Consider switching over a tuple of (diagnostic, expectedResult) to make this easier to understand. #Resolved
| [Fact] | ||
| public void ConstantFolding() | ||
| { | ||
| constantFolding("nint", "const nint A = -2147483648;", "+A", "-2147483648"); |
There was a problem hiding this comment.
Can you please add some comments clarifying the choices of numbers? I assume that these are max values of something, but I don't have the max values of various integer types memorized. #Resolved
|
Done review pass (commit 4) |
| } | ||
| catch (OverflowException) | ||
| { | ||
| Error(diagnostics, ErrorCode.ERR_CheckedOverflow, syntax); |
There was a problem hiding this comment.
Error(diagnostics, ErrorCode.ERR_CheckedOverflow, syntax); [](start = 16, length = 58)
I would expect this the error reported conditionally. Only when CheckOverflowAtCompileTime is true. An error in unchecked context is unexpected, we simply should evaluate the operation at runtime. #Closed
| return valueLeft.UInt32Value << valueRight.Int32Value; | ||
| case BinaryOperatorKind.ULongLeftShift: | ||
| return valueLeft.UInt64Value << valueRight.Int32Value; | ||
| case BinaryOperatorKind.IntRightShift: | ||
| case BinaryOperatorKind.NIntRightShift: | ||
| return valueLeft.Int32Value >> valueRight.Int32Value; |
There was a problem hiding this comment.
return valueLeft.Int32Value >> valueRight.Int32Value; [](start = 20, length = 53)
Similar here #Closed
| @@ -1311,13 +1311,42 @@ private static object FoldDecimalBinaryOperators(BinaryOperatorKind kind, Consta | |||
| return null; | |||
| } | |||
|
|
|||
| private static object FoldNativeIntegerBinaryOperator(BinaryOperatorKind kind, ConstantValue valueLeft, ConstantValue valueRight) | |||
There was a problem hiding this comment.
FoldNativeIntegerBinaryOperator [](start = 30, length = 31)
Given the name, one might assume this method handles all operators, but this is not the case. Consider reflecting this fact in the name. FoldNativeIntegerOverflowingBinaryOperator or something similar. #Closed
| } | ||
| catch (OverflowException) | ||
| { | ||
| Error(diagnostics, ErrorCode.ERR_CheckedOverflow, syntax); |
There was a problem hiding this comment.
Error(diagnostics, ErrorCode.ERR_CheckedOverflow, syntax); [](start = 16, length = 58)
It feels like we should be paying attention to CheckOverflowAtCompileTime here. #Closed
| @@ -2559,6 +2645,23 @@ private static object FoldCheckedIntegralUnaryOperator(UnaryOperatorKind kind, C | |||
| return null; | |||
| } | |||
|
|
|||
| private static object FoldNativeIntegerUnaryOperator(UnaryOperatorKind kind, ConstantValue value) | |||
There was a problem hiding this comment.
FoldNativeIntegerUnaryOperator [](start = 30, length = 30)
Same comment for the method name as for the one handling binary operators #Closed
| return -value.Int32Value; | ||
| case UnaryOperatorKind.NIntBitwiseComplement: | ||
| case UnaryOperatorKind.NUIntBitwiseComplement: | ||
| throw new OverflowException(); |
There was a problem hiding this comment.
throw new OverflowException(); [](start = 24, length = 30)
This is unexpected. These operators do not overflow, we simply unable to fold, therefore should return null. #Closed
|
Done with review pass (iteration 8), tests are not looked at. #Closed |
| } | ||
| else | ||
| { | ||
| return null; |
There was a problem hiding this comment.
return null [](start = 20, length = 11)
This does not look correct. Nowhere else in the language do we turn a constant expression into a non-constant expression because we don't know the result. For the other cases (in an unchecked context) we currently produce an "unspecified" constant result. An expression specified to be a constant expression cannot fail to be constant-folded, though the result may be unspecified. Please open an LDM issue about whether an expression that triggers this behavior should turn into a non-constant expression, produce an unspecified result, or result in an error.
There was a problem hiding this comment.
The return null; results in an error for G1 below, but means the value G2 will be calculated at runtime.
const nint F = int.MinValue;
const nint G1 = unchecked(-F); // CS0133: The expression must be constant
nint G2 = unchecked(-F); // calculated at runtimeIn reply to: 389839506 [](ancestors = 389839506)
There was a problem hiding this comment.
| {(IntPtr.Size == 4 ? "System.OverflowException" : "2147483648")} | ||
| 0 | ||
| {(IntPtr.Size == 4 ? "System.OverflowException" : "0")}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
} [](start = 4, length = 1)
Unless we already have them, it will probably be useful to have tests that target ExecutionArchitecture.x64 and verify runtime behavior for unchecked/never-checked operators for scenarios when folding cannot be performed at compile time. #Closed
There was a problem hiding this comment.
There was a problem hiding this comment.
Tests are executed with x86 and x64.
And we have ability to detect the platform and test accordingly. See NestedIfStatements unit-test, for example.
In reply to: 389936772 [](ancestors = 389936772,389935658)
There was a problem hiding this comment.
Perhaps I misunderstood the response. Are you saying we already have tests like that?
In reply to: 389941527 [](ancestors = 389941527,389936772,389935658)
There was a problem hiding this comment.
There are a number of tests that test differences in behavior for 32-bit and 64-bit. See use of IntPtr.Size in this file.
In reply to: 389982721 [](ancestors = 389982721,389941527,389936772,389935658)
| binaryOperator("bool", "!=", "nuint", "0", "nuint", uintMaxValue, "True"); | ||
| binaryOperator("bool", "!=", "nuint", uintMaxValue, "nuint", uintMaxValue, "False"); | ||
|
|
||
| binaryOperator("nint", "<<", "nint", intMinValue, "int", "0", intMinValue); |
There was a problem hiding this comment.
"<<" [](start = 35, length = 4)
It feels like for some values result should be platform dependent.
There was a problem hiding this comment.
| unaryOperator("nuint", "+", uintMaxValue, uintMaxValue); | ||
|
|
||
| unaryOperator("nint", "-", "-1", "1"); | ||
| unaryOperatorCheckedOverflow("nint", "-", intMinValue, IntPtr.Size == 4 ? "-2147483648" : "2147483648"); |
There was a problem hiding this comment.
Still a bunch of magic constants in the tests. In this line I see intMinValue and what I assume is intMaxValue+1?
|
Done with review pass (iteration 10) |
Test plan #38821