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

Use references to stack values in MCOPY #648

Merged
merged 1 commit into from
May 26, 2023
Merged

Conversation

gumb0
Copy link
Member

@gumb0 gumb0 commented May 15, 2023

pop() returns a reference and we don't need to make an extra copy.

Previous discussion: #629 (review)

@codecov
Copy link

codecov bot commented May 15, 2023

Codecov Report

Merging #648 (68bef8f) into master (d5f1f71) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #648   +/-   ##
=======================================
  Coverage   97.32%   97.32%           
=======================================
  Files          80       80           
  Lines        7739     7739           
=======================================
  Hits         7532     7532           
  Misses        207      207           
Flag Coverage Δ
statetests 74.65% <ø> (+0.10%) ⬆️
unittests 94.83% <100.00%> (ø)

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

Impacted Files Coverage Δ
lib/evmone/instructions.hpp 100.00% <100.00%> (ø)

@gumb0 gumb0 requested a review from chfast May 15, 2023 09:30
@chfast
Copy link
Member

chfast commented May 15, 2023

This disabled inlining of evmone::instr::core::mcopy.

@gumb0
Copy link
Member Author

gumb0 commented May 15, 2023

This disabled inlining of evmone::instr::core::mcopy.

Is it strong enough reason to do extra copying of 3 u256s and be inconsistent with other instruction implementations?

@chfast chfast mentioned this pull request May 24, 2023
@chfast chfast force-pushed the mcopy-args-reference branch 2 times, most recently from 42ce6d7 to 4e6e7d3 Compare May 24, 2023 10:25
@chfast chfast force-pushed the mcopy-args-reference branch from 4e6e7d3 to 68bef8f Compare May 26, 2023 10:28
@chfast chfast merged commit daf1fcc into master May 26, 2023
@chfast chfast deleted the mcopy-args-reference branch May 26, 2023 10:51
@yperbasis yperbasis added the Cancun Changes for Cancun Ethereum spec revision label Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cancun Changes for Cancun Ethereum spec revision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants