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

Convert evmc::hex library into single hex.hpp header #643

Merged
merged 3 commits into from
May 20, 2022
Merged

Conversation

chfast
Copy link
Collaborator

@chfast chfast commented May 12, 2022

No description provided.

@chfast chfast force-pushed the hex_inline branch 3 times, most recently from 9e9932c to 1fa70ca Compare May 12, 2022 14:19
@codecov
Copy link

codecov bot commented May 12, 2022

Codecov Report

Merging #643 (3016bf6) into master (a1d2845) will increase coverage by 0.00%.
The diff coverage is 98.09%.

@@           Coverage Diff           @@
##           master     #643   +/-   ##
=======================================
  Coverage   92.86%   92.86%           
=======================================
  Files          24       23    -1     
  Lines        3574     3546   -28     
  Branches      367      375    +8     
=======================================
- Hits         3319     3293   -26     
+ Misses        146      144    -2     
  Partials      109      109           

@chfast chfast marked this pull request as ready for review May 12, 2022 15:38
@chfast chfast requested review from axic, gumb0 and yperbasis May 12, 2022 15:54
@chfast chfast force-pushed the hex_inline branch 2 times, most recently from 329e36d to 1366a05 Compare May 13, 2022 14:37
include/evmc/hex.hpp Outdated Show resolved Hide resolved
include/evmc/hex.hpp Outdated Show resolved Hide resolved
include/evmc/hex.hpp Outdated Show resolved Hide resolved
lib/tooling/run.cpp Outdated Show resolved Hide resolved
int b = empty_byte_mark;
for (const auto h : hex)
{
if (isspace(h))
Copy link
Member

Choose a reason for hiding this comment

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

Why allow space here? Why not allow space in the prefix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is practical as a separator for readability or when loading from files (where usually there is the trailing new line character).

Not sure what you mean "in the prefix". Like 0 x 01?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

include/evmc/hex.hpp Outdated Show resolved Hide resolved
EXPECT_EQ(ec2.category(), hex_category());
for (int i = int{std::numeric_limits<char>::min()}; i <= std::numeric_limits<char>::max(); ++i)
{
switch (const auto c = static_cast<char>(i); c)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add std::isspace(c) == evmc::internal_hex::isspace(c) too? Probably not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, that's actually much better idea than my test.

include/evmc/hex.hpp Outdated Show resolved Hide resolved
@chfast chfast force-pushed the hex_inline branch 3 times, most recently from f09a7da to a476d6e Compare May 20, 2022 08:44
case '\n':
case '\r':
case '\t':
case '\v':
Copy link
Member

Choose a reason for hiding this comment

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

I actually wonder if isspace should be used in the hex parser or just checking for space would be enough?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need at least , \r, \n.

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.

Looks good to me otherwise.

This replaces std::error_code and exceptions with std::optional<bytes>
to inform about invalid hex input.
@chfast chfast merged commit 5e3d163 into master May 20, 2022
@chfast chfast deleted the hex_inline branch May 20, 2022 13:12
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.

None yet

3 participants