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: Remove duplicated type section size check #1137

Merged
merged 1 commit into from
Feb 20, 2025

Conversation

chfast
Copy link
Member

@chfast chfast commented Feb 19, 2025

The check that type section size matches the number of code sections is done twice in EOF validation:

  • in validate_section_headers()
  • in validate_header() which always calls validate_section_headers() so this check never fails.

I decided to leave the check in validate_header() because it better fits there.

@chfast chfast added the EOF label Feb 19, 2025
The check that type section size matches the number of code sections
is done twice in EOF validation:
- in `validate_section_headers()`
- in `validate_header()` which always calls `validate_section_headers()`
  so this check never fails.

I decided to leave the check in `validate_header()` because it better
fits there.
@chfast chfast force-pushed the eof/duplicated_type_section_size_check branch from 1e500d7 to 466fb42 Compare February 19, 2025 20:39
@chfast chfast requested review from gumb0 and pdobacz February 19, 2025 20:43
Copy link

codecov bot commented Feb 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.50%. Comparing base (d62977d) to head (466fb42).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1137   +/-   ##
=======================================
  Coverage   94.50%   94.50%           
=======================================
  Files         167      167           
  Lines       18202    18199    -3     
=======================================
- Hits        17201    17199    -2     
+ Misses       1001     1000    -1     
Flag Coverage Δ
eof_execution_spec_tests 15.33% <ø> (-0.01%) ⬇️
ethereum_tests 28.31% <ø> (-0.01%) ⬇️
ethereum_tests_silkpre 20.83% <ø> (+<0.01%) ⬆️
execution_spec_tests 21.09% <ø> (+<0.01%) ⬆️
unittests 89.76% <ø> (+<0.01%) ⬆️

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

Files with missing lines Coverage Δ
lib/evmone/eof.cpp 85.88% <ø> (+0.09%) ⬆️

Copy link
Contributor

@pdobacz pdobacz left a comment

Choose a reason for hiding this comment

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

I was trying hard to remember the historical reasons behind this, but couldn't. lgtm

@chfast
Copy link
Member Author

chfast commented Feb 20, 2025

I was trying hard to remember the historical reasons behind this, but couldn't. lgtm

Added here: https://github.com/ethereum/evmone/pull/1034/files#diff-6c68f99275b828a78c2791fee776edf5c210a321c916e8afc58689b1d4d5762dR771

@chfast chfast merged commit 9e8ddb2 into master Feb 20, 2025
25 checks passed
@chfast chfast deleted the eof/duplicated_type_section_size_check branch February 20, 2025 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants