-
Notifications
You must be signed in to change notification settings - Fork 11.3k
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
[move] support const in const #14223
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Ignored Deployments
|
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.
Overall, looks great!
Thoughts on how difficult this would be to add for public(package)
constants (plus adding support for public(package)
constants :))
let name = match e_ { | ||
E::Constant(name) => name, | ||
_ => unreachable!(), |
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.
minor nit, let else might read better here
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.
There are several other arms of this match that use a similar let pattern. Should I rewrite them as well?
external-crates/move/move-compiler/src/cfgir/optimize/constant_fold.rs
Outdated
Show resolved
Hide resolved
external-crates/move/move-compiler/tests/move_2024/typing/const_in_const_circular.exp
Outdated
Show resolved
Hide resolved
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.
Overall looks good to me. A couple small nits.
Could you also add a couple valid tests to make sure constants are actually folded? Currently I'm just seeing the change to the already existing cyclic const test.
let name = if let E::Constant(name) = e_ { | ||
name | ||
} else { | ||
unreachable!() | ||
}; |
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.
Nit: a perfect use-case for let-else IMO
let name = if let E::Constant(name) = e_ { | |
name | |
} else { | |
unreachable!() | |
}; | |
let E::Constant(name) = e_ else { | |
unreachable!() | |
}; |
@tzakian I'll fix the nits when I get back to the office, but -- doesn't the transactional test I posted do the correct thing for testing they are actually folded? |
Egads you're totally right. For some reason my eyes just went to the expected value files and errors, and I didn't see those. But yes, those are exactly the correct tests so please disregard :) |
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.
Looks good! Just make sure you pick up @tzakian's spelling and style comments :)
if let Some(mident) = &self.current_module { | ||
self.module_info(mident).package |
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.
nit, self.current_module.as_ref().map
## Description Support constants in constants. ## Test Plan Added new tests --- If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process. ### Type of Change (Check all that apply) - [ ] protocol change - [x] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes The error for using const-in-const changed to point at the feature gating.
Description
Support constants in constants.
Test Plan
Added new tests
If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.
Type of Change (Check all that apply)
Release notes
The error for using const-in-const changed to point at the feature gating.