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

refactor(primitives): improve readability of Env.validate_tx() #924

Merged
merged 1 commit into from
Dec 30, 2023

Conversation

IaroslavMazur
Copy link
Contributor

The optimization comes from the fact that the respective variables are only created if they're, actually, going to be used after that (instead of being untouched due to an early return).

@rakita
Copy link
Member

rakita commented Dec 25, 2023

Would not merge this, the compiler is good enough to optimize it and not touching variables is a uncommon path.

@rakita rakita closed this Dec 25, 2023
@IaroslavMazur
Copy link
Contributor Author

Would not merge this, the compiler is good enough to optimize it and not touching variables is a uncommon path.

Didn't know the compiler is looking for ways to make such optimizations, as well. Good to know!
In that case, these changes don't, actually, "optimize" things.

However, I believe that the proposed changes increase code readability.

@rakita
Copy link
Member

rakita commented Dec 27, 2023

e proposed changes increase code readability.

Would agree with readability, I would even remove the temp variable and use functions directly.

@IaroslavMazur
Copy link
Contributor Author

e proposed changes increase code readability.

Would agree with readability, I would even remove the temp variable and use functions directly.

Should I, then, reopen the PR, removing the temporary variables?

@rakita rakita reopened this Dec 30, 2023
@rakita
Copy link
Member

rakita commented Dec 30, 2023

e proposed changes increase code readability.

Would agree with readability, I would even remove the temp variable and use functions directly.

whatever is easier for you, if you want to do it in this PR just edit the title

@IaroslavMazur IaroslavMazur force-pushed the iaro/optimize-env-validate-tx branch from 83f51f1 to 4e4ce0d Compare December 30, 2023 15:54
@IaroslavMazur IaroslavMazur changed the title perf(primitives): optimize Env.validate_tx() refactor(primitives): improve readability of Env.validate_tx() Dec 30, 2023
@IaroslavMazur
Copy link
Contributor Author

whatever is easier for you, if you want to do it in this PR just edit the title

Done 🤝

Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

lgtm!

@rakita rakita merged commit 75e9567 into bluealloy:main Dec 30, 2023
@github-actions github-actions bot mentioned this pull request Jan 12, 2024
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