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

Fix warnings for signedness #312

Merged
merged 1 commit into from
Aug 19, 2018
Merged

Fix warnings for signedness #312

merged 1 commit into from
Aug 19, 2018

Conversation

axic
Copy link
Member

@axic axic commented Aug 15, 2018

Part of #311.

@axic axic requested a review from chfast August 15, 2018 21:04
Copy link
Collaborator

@chfast chfast left a comment

Choose a reason for hiding this comment

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

This goes in wrong direction. Unsigned types should be avoided. They are only good for bitfields and bit manipulation.

src/eei.cpp Outdated
@@ -370,20 +370,20 @@ inline int64_t maxCallGas(int64_t gas) {

takeInterfaceGas(GasSchedule::base);

return Literal(static_cast<uint32_t>(m_code.size()));
return Literal(uint32_t(m_code.size()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do not hide a cast.

@axic
Copy link
Member Author

axic commented Aug 15, 2018

Unsigned types should be avoided. They are only good for bitfields and bit manipulation.

I don't get it, why?

@chfast
Copy link
Collaborator

chfast commented Aug 15, 2018

They are only good for bitfields and bit manipulation.

@axic
Copy link
Member Author

axic commented Aug 15, 2018

Still, why?

@chfast
Copy link
Collaborator

chfast commented Aug 15, 2018

I had to look it up. Most of the arguments are listed here (read all section): https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#arithmetic.

I should add that you can detect signed overflow with Undefined Behavior Sanitizer (with the newest version you can also detect unsigned overflows, but because they are valid in C++, you can get false positives). Together with fuzz testing this gives incredible results.

@codecov-io
Copy link

codecov-io commented Aug 19, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@cd6782a). Click here to learn what that means.
The diff coverage is 91.07%.

@@            Coverage Diff            @@
##             master     #312   +/-   ##
=========================================
  Coverage          ?   67.55%           
=========================================
  Files             ?        5           
  Lines             ?      860           
  Branches          ?      122           
=========================================
  Hits              ?      581           
  Misses            ?      251           
  Partials          ?       28

@axic
Copy link
Member Author

axic commented Aug 19, 2018

As discussed the specification isn't clear what the types of these are, but implicitly it feels they are defined as unsigned in the EEI. Added an issue to review that: ewasm/design#132

Merging this change as it doesn't changes semantics.

@axic axic merged commit a19524c into master Aug 19, 2018
@axic axic removed the in progress label Aug 19, 2018
@axic axic deleted the signedness branch August 19, 2018 19:55
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