a collection of simple const changes by npmccallum · Pull Request #146600 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation16 Commits1 Checks11 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.
cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.
labels
r? @tgross35
rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This consists mostly of derive changes or simple impl changes.
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.
Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing anything const in this module is a bit confusing as we can't actually make anything interesting const. I'd rather not change anything in the fmt machinery that isn't clearly connected to a use case
| )+) => {$( |
|---|
| #[derive(Clone, Copy, Eq)] |
| #[derive(Copy)] |
| #[derive_const(Clone, Eq)] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do that after the pattern types refactor lands. Otherwise I may just have to temporarily remove it again
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ironically, this is the only change I actually need to constify all the slice and array types and their dependents. Are you sure this is blocked? I'll file a separate PR with that enablement path made obvious and request you as reviewer.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I may have resolved this problem.
| #[derive_const(Clone, PartialEq, Eq, PartialOrd, Ord)] |
|---|
| #[lang = "coroutine_state"] |
| #[unstable(feature = "coroutine_trait", issue = "43122")] |
| pub enum CoroutineState<Y, R> { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coroutines have no support for getting polled in const code. Let's pair this constification with actual coroutine constifications
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are many other types that don't really make sense in const contexts yet that you constified impls on. I'm not sure how useful this is. I guess we do need PRs like this at some point just to finish constification. But rn it just feels like a hard to review PR that is easy to write
View changes since this review
There are many other types that don't really make sense in const contexts yet that you constified impls on. I'm not sure how useful this is. I guess we do need PRs like this at some point just to finish constification. But rn it just feels like a hard to review PR that is easy to write
That's a fair critique. I was honestly trying to fix some of the core paths and realized that there was a lot that could be const that nobody had done yet. So I was just trying to be helpful.
Labels
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.