Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change m_bytes to unsigned in FixedBytesType #4038

Merged
merged 3 commits into from
May 3, 2018
Merged

Conversation

jdlee23
Copy link
Contributor

@jdlee23 jdlee23 commented May 1, 2018

Fixes #3965.
Closes #4036.
Closes #4010.

🙏 sorry i left a mess on that other PR.

any chance i could poke one of yall to help me set up the local testing environment? realized changing numBytes might lead me down a bit of a rabbit hole, and i didn't want to continuously push! i'll buy yall a round of beers

@axic
Copy link
Member

axic commented May 1, 2018

You can also click on the Circleci log and find out the reason for failures:

/root/project/libsolidity/ast/Types.cpp: In member function 'virtual bool dev::solidity::RationalNumberType::isImplicitlyConvertibleTo(const dev::solidity::Type&) const':
/root/project/libsolidity/ast/Types.cpp:876:38: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
     return fixedBytes.numBytes() * 8 >= integerType()->numBits();
            ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~

Please adjust the code for it to compile.

@jdlee23
Copy link
Contributor Author

jdlee23 commented May 1, 2018

cheers @axic

I updated the PR to fix any compilation errors. I don't mind not receiving any bounty for this, although I'd love to contribute more slowly on the easier tickets.

Copy link
Member

@axic axic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase this over #4036.

@@ -365,7 +365,7 @@ class FixedPointType: public Type
};
virtual Category category() const override { return Category::FixedPoint; }

explicit FixedPointType(int _totalBits, int _fractionalDigits, Modifier _modifier = Modifier::Unsigned);
explicit FixedPointType(unsigned _totalBits, int _fractionalDigits, Modifier _modifier = Modifier::Unsigned);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you chose to make totalBits unsigned while leaving fractionalDigits signed?

Copy link
Contributor Author

@jdlee23 jdlee23 May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just changed everything that m_bytes was connected to from int to unsigned along with warning/errors until the compiler was happy. Should I update _factionalDigits to unsigned as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you rebase it over #4036 then you don't need to.

@chriseth
Copy link
Contributor

chriseth commented May 2, 2018

There is another int in CompilerUtils.cpp:691 and CompilerUtils.cpp:799, but then I think that's almost it!

chriseth
chriseth previously approved these changes May 3, 2018
@chriseth chriseth dismissed their stale review May 3, 2018 06:30

Test errors.

@chriseth
Copy link
Contributor

chriseth commented May 3, 2018

Sorry, but there is at least one compiler error:

/root/project/libsolidity/ast/Types.cpp:603:5: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits]
   0 <= m_fractionalDigits && m_fractionalDigits <= 80,
   ~~^~~~

@jdlee23
Copy link
Contributor Author

jdlee23 commented May 3, 2018

nice catch

@@ -600,7 +600,7 @@ FixedPointType::FixedPointType(unsigned _totalBits, unsigned _fractionalDigits,
{
solAssert(
8 <= m_totalBits && m_totalBits <= 256 && m_totalBits % 8 == 0 &&
0 <= m_fractionalDigits && m_fractionalDigits <= 80,
0 < m_fractionalDigits && m_fractionalDigits <= 80,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I'm not sure if we actually force m_fractionalDigits to be nonzero somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I keep it as an int?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, just remove the lower bound check.

@chriseth
Copy link
Contributor

chriseth commented May 3, 2018

Please incorporate the last commit into the appropriate bigger one.

chriseth
chriseth previously approved these changes May 3, 2018
@jdlee23
Copy link
Contributor Author

jdlee23 commented May 3, 2018

@axic does this look good?

@chriseth
Copy link
Contributor

chriseth commented May 3, 2018

We have a weird test failure:

ASSERTION FAILURE:
- file   : SolidityEndToEndTest.cpp
- line   : 3863
- message: Invalid encoded data
   Result                                                           Expectation
 X ................................................................ .........................da99.5c69723ff2b35428c8.cde458eff965dee

restarted the test.

@jdlee23
Copy link
Contributor Author

jdlee23 commented May 3, 2018

thanks @chriseth !

@axic axic merged commit a244f1a into ethereum:develop May 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants