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.

Sign up for GitHub

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

Jump to bottom

Stabilize const_refs_to_static #129759

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Stabilize const_refs_to_static #129759

wants to merge 9 commits into from
+154 −442

Conversation

dingxiangfei2009
Copy link
Contributor

@dingxiangfei2009 dingxiangfei2009 commented Aug 29, 2024 •
edited by workingjubilee
Loading

Close #128183
Tracked by #119618
cc @nikomatsakis

Meanwhile, I am cooking a sub-section in the language reference.

@rustbot
Copy link
Collaborator

rustbot commented Aug 29, 2024

r? @petrochenkov

rustbot has assigned @petrochenkov.
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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 29, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor

Blocked on #129472.
@rustbot blocked

@rustbot rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 30, 2024
@bors

This comment was marked as resolved.

Sign in to view
@traviscross
Copy link
Contributor

@rustbot ready

(#129472 has been merged.)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Sep 6, 2024
@petrochenkov
Copy link
Contributor

Looks like FCP in #128183 (comment) has completed yesterday.

@petrochenkov
Copy link
Contributor

The implementation seems trivial, so I can review and approve, but if you want someone from the const eval group to do that, feel free to reassign.

@petrochenkov
Copy link
Contributor

This also needs a rebase and a fix for the error index.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 7, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot
Copy link
Collaborator

rustbot commented Sep 11, 2024

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@dingxiangfei2009 dingxiangfei2009 mentioned this pull request Sep 11, 2024
Open
@dingxiangfei2009
Copy link
Contributor Author

@rustbot ready

  • A test stderr is reverted
  • Error code document is updated
  • The ref book PR is ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 11, 2024
@dingxiangfei2009
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 16, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member

RalfJung commented Sep 16, 2024 •
edited
Loading

Please avoid unnecessary (i.e., not conflict-induced) rebases over master. Use git rebase --keep-base <master branch> when rebasing to squash. That way, it remains somewhat possible to check in github the diff of what you did. If you rebase with a base change, the poor handling of force pushes in github means there's no way to get a nice diff, and the only option to see what changed is a complete re-review.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@traviscross traviscross mentioned this pull request Aug 28, 2024
Open
6 tasks
@dingxiangfei2009
Copy link
Contributor Author

@rustbot ready

  • It turns out that there is a compiletest issue. Tests in tests/ui/asm are run with --emit=metadata. In this case, probably not all const eval check is invoked for global_asm!. Forcing the test to check items thoroughly with --emit=asm allows us to recover all the errors.
  • One test is dropped.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Sep 19, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@workingjubilee
Copy link
Member

Ah, so it was concerned about the reference, not the integer. Apologies for misleading you. I don't know a way to deal with that aside from normalization.

workingjubilee
workingjubilee reviewed Sep 19, 2024
View reviewed changes
tests/ui/consts/issue-17718-const-bad-values.rs
Copy link
Member

Choose a reason for hiding this comment

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

At the top of the file, I believe:

//@ normalize-stderr-32bit: "the raw bytes of the constant \(size: 4, align: 4\)" -> "the raw bytes of the constant \(size: $$PTR, align: $$PTR\)"
//@ normalize-stderr-64bit: "the raw bytes of the constant \(size: 8, align: 8\)" -> "the raw bytes of the constant \(size: $$PTR, align: $$PTR\)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right!

Copy link
Member

Choose a reason for hiding this comment

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

Or just use a regex like \d+ to avoid hard-coding some numbers

Copy link
Member

@workingjubilee workingjubilee Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think these numbers will change much, but you're right that that's nicer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to regex \d+

@dingxiangfei2009
Copy link
Contributor Author

To make sure that the tests still run on the critical architectures, I am going to

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 20, 2024
@bors
Auto merge of rust-lang#129759 - dingxiangfei2009:stabilize-const-ref…
47e9197 
…s-to-static, r=<try>

Stabilize `const_refs_to_static`

Meanwhile, I am cooking a sub-section in the language reference.
@bors
Copy link
Contributor

bors commented Sep 20, 2024

⌛ Trying commit 539b624 with merge 47e91977e7dcb439530140bf1a5a3e6c37e8b9fa...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

@workingjubilee workingjubilee workingjubilee left review comments

@petrochenkov petrochenkov petrochenkov approved these changes

Assignees

@RalfJung RalfJung

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proposal: stabilize const_refs_to_static
8 participants
@dingxiangfei2009 @rustbot @rust-log-analyzer @petrochenkov @bors @traviscross @workingjubilee @RalfJung