-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Add Yul EVM version "current" #15639
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
Conversation
test/libyul/evmCodeTransform/stackReuse/function_argument_reuse_without_retparams.yul
Outdated
Show resolved
Hide resolved
liblangutil/EVMVersion.h
Outdated
@@ -85,6 +87,8 @@ class EVMVersion: | |||
|
|||
static std::optional<EVMVersion> fromString(std::string const& _version) | |||
{ | |||
if (_version == "current") | |||
return current(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this exposes the new alias everywhere, including the --evm-version
option on the CLI. That wasn't really my intention here. I was thinking about just making it available in the EVMVersion
setting in tests as a special value.
I would not be against having such an alias. Even internally the fact that EVMVersion{}
means automatically the latest version may not be obvious (it wasn't to me at first), even if it's convenient. But it's a bigger change we should discuss more broadly first. It would also need a changelog entry and should be documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That default ctor means current version was also anything but obvious to me before looking at the code. But you are right of course, this is a change that is outside-facing. Let's discuss it next week perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. In this PR I'd still limit it to tests though so that we can merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
16843ff
to
d9e5338
Compare
d9e5338
to
ee1aae5
Compare
2552bea
to
2064442
Compare
see this discussion: #15631 (comment)