Make ConstantEvaluator truncate the result of shift operations#16597
Make ConstantEvaluator truncate the result of shift operations#16597matheusaaguiar wants to merge 5 commits intodevelopfrom
ConstantEvaluator truncate the result of shift operations#16597Conversation
| )) | ||
| { | ||
| TypedValue convertedValue = convertType(*value, *resultType); | ||
| TypedValue convertedValue = TokenTraits::isShiftOp(_operation.getOperator()) ? |
There was a problem hiding this comment.
I guess it could be done only for left shift.
|
Not sure about a Changelog entry... |
matheusaaguiar
force-pushed
the
ConstantEvaluator-truncate-shiftL-value
branch
from
86ac941 to
a2cb1da
Compare
There was a problem hiding this comment.
Some code structure feedback and questions. Overall looks correct to me. I think BitNot has similar issues. For example ~uint8(0) and ~uint8(-128) both fail constant eval. If you agree, this could be folded into this pr or be a follow up.
Not sure about a Changelog entry...
It's user-facing, isn't it? So imo having an entry is the right move.
| solAssert(integerType); | ||
|
|
||
| solAssert(_value.denominator() == 1); | ||
| bigint integerValue = _value.numerator() / _value.denominator(); |
There was a problem hiding this comment.
| bigint integerValue = _value.numerator() / _value.denominator(); | |
| bigint integerValue = _value.numerator(); |
you assert that denom is 1 right above
| auto const* integerType = dynamic_cast<IntegerType const*>(&_type); | ||
| solAssert(integerType); | ||
|
|
||
| solAssert(_value.denominator() == 1); |
There was a problem hiding this comment.
why can we assume that, because it's an IntegerType? Is rational always in reduced/normalized form?
There was a problem hiding this comment.
Because it's IntegerType. IIRC, in comptime, it is always normalized. I will confirm this.
| integerValue = integerValue & mask; | ||
|
|
||
| // extend sign if needed | ||
| if (integerType->isSigned() && (integerValue & sign)) |
There was a problem hiding this comment.
| if (integerType->isSigned() && (integerValue & sign)) | |
| bool const isNegative = integerType->isSigned() && boost::multiprecision::bit_test(integerValue, numBits - 1); | |
| if (isNegative) |
then the sign mask above isn't needed anymore
| TypedValue convertedValue = TokenTraits::isShiftOp(_operation.getOperator()) ? | ||
| cleanupValue(*value, *resultType) : | ||
| convertType(*value, *resultType); |
There was a problem hiding this comment.
Personally I'd rather introduce a helper rational truncateToType(rational const& _value, IntegerType const& _type) and then do the conversion directly here. that would avoid mixing concerns where, right now, cleanupValue both converts and truncates. I imagine it could read something like
| TypedValue convertedValue = TokenTraits::isShiftOp(_operation.getOperator()) ? | |
| cleanupValue(*value, *resultType) : | |
| convertType(*value, *resultType); | |
| rational resultValue = *value; | |
| if (isShiftOp) | |
| if (auto const* integerType = dynamic_cast...) | |
| resultValue = truncateToType(resultValue, *integerType); | |
| TypedValue convertedValue = convertType(resultValue, *resultType); |
There was a problem hiding this comment.
Yeah, was concerned too about cleanupValue calling convertType at the end...will implement your suggestion.
matheusaaguiar
force-pushed
the
ConstantEvaluator-truncate-shiftL-value
branch
from
2963520 to
f695046
Compare
matheusaaguiar
force-pushed
the
ConstantEvaluator-truncate-shiftL-value
branch
from
f695046 to
36f5fa3
Compare
2 participants
Fix #16596.
Spotted in #16456 (comment).