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

eof: Change handling of errors from pair<val, err> to variant<val, err> #572

Merged
merged 1 commit into from
Feb 21, 2023

Conversation

axic
Copy link
Member

@axic axic commented Feb 20, 2023

Pulled out of #563, but not sure who the author is.

@codecov
Copy link

codecov bot commented Feb 20, 2023

Codecov Report

Merging #572 (e4278a9) into master (09851dd) will increase coverage by 0.00%.
The diff coverage is 95.65%.

❗ Current head e4278a9 differs from pull request most recent head 8c4c6c4. Consider uploading reports for the commit 8c4c6c4 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #572   +/-   ##
=======================================
  Coverage   96.94%   96.95%           
=======================================
  Files          67       67           
  Lines        6228     6232    +4     
=======================================
+ Hits         6038     6042    +4     
  Misses        190      190           
Flag Coverage Δ
blockchaintests 75.48% <0.00%> (ø)
statetests 70.94% <0.00%> (ø)
unittests 92.90% <95.65%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/evmone/eof.cpp 80.41% <95.65%> (+0.56%) ⬆️

@@ -186,7 +189,12 @@ EOFValidationError validate_eof(evmc_revision rev, bytes_view container) noexcep
{
if (rev < EVMC_CANCUN)
return EOFValidationError::eof_version_unknown;
return validate_eof1(rev, container).second;

const auto header_or_error = validate_eof1(rev, container);
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems this function isn't even using the returned EOF1Header and so the entire deal with pair/variant seems useless.

Copy link
Member

Choose a reason for hiding this comment

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

For simplicity we can change validate_eof1() to return only error.

Copy link
Member

Choose a reason for hiding this comment

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

We can also make pubic validate_eof() return variant, in general it seems useful to be able to validate and parse in one go.

But currently I found that it could be useful only in eofparse (which does validate_eof() + read_valid_eof1_header() combo)

State test runner seems to do these two operations in different places.

@axic
Copy link
Member Author

axic commented Feb 20, 2023

@chfast @gumb0 @rodiazet it is draft because I'm unsure of usefulness, but please review.

@gumb0
Copy link
Member

gumb0 commented Feb 21, 2023

Have you seen 6acd9d3 ?

Ah I see it's the same commit (I was the author)

@axic
Copy link
Member Author

axic commented Feb 21, 2023

Have you seen 6acd9d3 ?

Ah I see it's the same commit (I was the author)

I actually duplicated the change from the final diff and was lazy to find the actual commit. Will change the author then.

This helps to reduce stack usage of large functions like validate_eof_headers().

Co-authored-by: Alex Beregszaszi <alex@rtfs.hu>
@axic axic marked this pull request as ready for review February 21, 2023 12:19
@axic axic requested review from gumb0, chfast and rodiazet February 21, 2023 12:19
@axic axic merged commit 8e22572 into master Feb 21, 2023
@axic axic deleted the eof-errors branch February 21, 2023 13: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.

2 participants