RationalNumberType::isImplicitlyConvertibleTo Refactor.#4304
RationalNumberType::isImplicitlyConvertibleTo Refactor.#4304khanrn wants to merge 9 commits intoargotorg:developfrom
Conversation
|
I think there is an issue with |
erak
left a comment
There was a problem hiding this comment.
@rnaby Please also check the syntax tests (can be found in test/libsolidity/syntaxTests) and the external tests. Some of them are currently failing because of your changes.
|
@bit-shift I think now it passes all the tests. 👍 Actually I saw early that it wasn't passing all the tests, but couldn't figured out where is the issue. Now did figure out the issue and fixed. 🙂 |
|
Now I think this pull request is ready to review again. 🙂 |
|
@rnaby Can you please squash the commits? |
430bf34 to
30a2d48
Compare
|
@bit-shift Commits has been squashed to a single commit. 🙂 |
libsolidity/ast/Types.cpp
Outdated
| else if (targetType.isSigned() && -m_value.numerator() <= (u256(1) << (targetType.numBits() - forSignBit))) | ||
| return true; | ||
| return false; | ||
| return m_value.numerator() <= (u256(-1) >> (256 - targetType.numBits() + forSignBit)) ? true : false; |
There was a problem hiding this comment.
Please remove the ? true : false, I think it complicates it and makes it harder to read (same below).
| return m_value.numerator() <= (u256(-1) >> (256 - targetType.numBits() + forSignBit)) ? true : false; | ||
| if (targetType.isSigned()) | ||
| return -m_value.numerator() <= (u256(1) << (targetType.numBits() - forSignBit)) ? true : false; | ||
| } |
There was a problem hiding this comment.
Please either keep the else if or add a return false here (preferrably add return false in any case because the function is rather long.
libsolidity/ast/Types.cpp
Outdated
| return -m_value.numerator() <= (u256(1) << (targetType.numBits() - forSignBit)) ? true : false; | ||
| } | ||
| else if (_convertTo.category() == Category::FixedPoint) | ||
| if (_convertTo.category() == Category::FixedPoint) |
There was a problem hiding this comment.
Could also be changed to a switch (_convertTo.category()) { ... }
libsolidity/ast/Types.cpp
Outdated
| } | ||
| else | ||
| return false; | ||
| return !isFractional() && integerType() ? fixedBytes.numBytes() * 8 >= integerType()->numBits() : false; |
There was a problem hiding this comment.
I actually find the previous version much better to read. It could be improved by removing the !, i.e.
if (isFractional())
return false;
else if (integerType())
return fixedBytes.numBytes() * 8 >= integerType()->numBits();
else
return false;
… ContractCompiler
This patch enfoces an error when it encounters an empty struct, effectively eliminating the deprecation warning. Also adjust 419_interface_structs to explicitely test for (non-empty) structs, as this behaviour "may" change in the future.
6b8d387 to
098f4da
Compare
|
Some merging issue happened, so will submit a different PR. |
|
@chriseth and @bit-shift A cleaned version of this PR is now at #4342 PR. |
No description provided.