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

Use wide pointers consistenly across the compiler #131202

Merged
merged 1 commit into from
Oct 4, 2024
Merged

Use wide pointers consistenly across the compiler #131202

merged 1 commit into from
Oct 4, 2024

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Oct 3, 2024 •
edited
Loading

This PR replace every use of "fat pointer" for the more recent "wide pointer" terminology.

As T-lang as preferred the "wide pointer" terminology, as can be seen on the last RFCs, on some lints, but also in the reference.

Currently we have a mix of both (including in error messages), which isn't great, but with this PR no more.

r? @jieyouxu (feel free to re-roll)

@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 Oct 3, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 3, 2024

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

@jieyouxu
Copy link
Member

jieyouxu commented Oct 3, 2024

@Urgau is there any kind of meeting or decision we can look at that shows T-lang preferred "wide pointers" over "fat pointers"?

@Urgau
Copy link
Member Author

Urgau commented Oct 3, 2024 •
edited
Loading

A part from the indirect usage described in the PR description, I don't think so (at least I haven't found one). I think it just sort of happened.

cc @traviscross

@jieyouxu
Copy link
Member

jieyouxu commented Oct 3, 2024 •
edited
Loading

A part from the indirect usage described in the PR description, I don't think so (at least I haven't found one). I think it just sort of happened.

Yeah personally I'm fine with sticking with one of "wide pointers" or "fat pointers" instead of mixing both terms, but I think we want this to be an explicit T-lang decision of migrating over to "wide pointers" with a clear definition in the reference that users can look at if they want clarity of what that concretely means.

For T-lang

Do we want to use "wide pointers" terminology over "fat pointers"? Is this term documented in the Reference?

Note that the Reference seems to use "wide pointers" in at least https://doc.rust-lang.org/reference/expressions/operator-expr.html?highlight=wide#semantics.

@rustbot label +I-lang-nominated +I-lang-easy-decision

If @traviscross knows that T-lang already prefers "wide pointers" over "fat pointers" then this is fine.

@jieyouxu jieyouxu added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-nominated Nominated for discussion during a lang team meeting. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2024
@Urgau Urgau force-pushed the wide-ptrs-compiler branch from 25a1bd3 to 2de4f21 Compare October 3, 2024 14:39
@Urgau
Copy link
Member Author

Urgau commented Oct 3, 2024

@jieyouxu actually, T-lang explicitly discussed fat vs wide for the naming of the ambiguous_wide_pointer_comparison lint in #117758 (comment), #117758 (comment), #117758 (comment) and they FCP-ed it

@jieyouxu
Copy link
Member

jieyouxu commented Oct 3, 2024

Ok great, in that case this seems fine.

@jieyouxu jieyouxu added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed I-lang-nominated Nominated for discussion during a lang team meeting. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy. T-lang Relevant to the language team, which will review and decide on the PR/issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Oct 3, 2024
jieyouxu
jieyouxu approved these changes Oct 3, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

LGTM with some nits, incl. consts and enums that still refers to FAT_PTR.

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/layout.rs Outdated Show resolved Hide resolved
@jieyouxu
Copy link
Member

jieyouxu commented Oct 3, 2024

@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 Oct 3, 2024
@Urgau Urgau force-pushed the wide-ptrs-compiler branch from 2de4f21 to c2f8071 Compare October 4, 2024 07:45
@Urgau
Copy link
Member Author

Urgau commented Oct 4, 2024

@rustbot ready

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 4, 2024
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 4, 2024
jieyouxu
jieyouxu approved these changes Oct 4, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks!

@jieyouxu
Copy link
Member

jieyouxu commented Oct 4, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 4, 2024

📌 Commit c2f8071 has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 4, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Oct 4, 2024

@bors rollup=maybe

@jieyouxu
Copy link
Member

jieyouxu commented Oct 4, 2024

Ah yeah.
@bors r-
Feel free to r=me after the tiny nit.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 4, 2024
@Urgau Urgau force-pushed the wide-ptrs-compiler branch from c2f8071 to 018ba05 Compare October 4, 2024 12:16
@Urgau
Copy link
Member Author

Urgau commented Oct 4, 2024

@bors r=@jieyouxu

@bors
Copy link
Contributor

bors commented Oct 4, 2024

📌 Commit 018ba05 has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 4, 2024
@GuillaumeGomez GuillaumeGomez mentioned this pull request Oct 4, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 4, 2024
@bors
Auto merge of rust-lang#131237 - GuillaumeGomez:rollup-il2i7z7, r=Gui…
c39f318 
…llaumeGomez

Rollup of 4 pull requests

Successful merges:

 - rust-lang#131034 (Implement RFC3695 Allow boolean literals as cfg predicates)
 - rust-lang#131202 (Use wide pointers consistenly across the compiler)
 - rust-lang#131230 (Enable `--no-sandbox` option by default for rustdoc GUI tests)
 - rust-lang#131232 (Week off of reviews to focus on docs)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ba94a2a into rust-lang:master Oct 4, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Oct 4, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 4, 2024
@rust-timer
Unrolled build for rust-lang#131202
1c20c31 
Rollup merge of rust-lang#131202 - Urgau:wide-ptrs-compiler, r=jieyouxu

Use wide pointers consistenly across the compiler

This PR replace every use of "fat pointer" for the more recent "wide pointer" terminology.

Since some time T-lang as preferred the "wide pointer" terminology, as can be seen on [the last RFCs](https://github.com/search?q=repo%3Arust-lang%2Frfcs+%22wide+pointer%22&type=code), on some [lints](https://doc.rust-lang.org/rustc/lints/listing/warn-by-default.html#ambiguous-wide-pointer-comparisons), but also in [the reference](https://doc.rust-lang.org/stable/reference/expressions/operator-expr.html?highlight=wide%20pointer#pointer-to-pointer-cast).

Currently we have a [mix of both](https://github.com/search?q=repo%3Arust-lang%2Frust+%22wide+pointer%22&type=code) (including in error messages), which isn't great, but with this PR no more.

r? `@jieyouxu` (feel free to re-roll)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lqd lqd lqd left review comments

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@jieyouxu jieyouxu

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.83.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Urgau @rustbot @jieyouxu @bors @lqd