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.

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

BindingAnnotation refactor #101241

Merged
merged 3 commits into from
Sep 6, 2022

Conversation

camsteffen
Copy link
Contributor

@camsteffen camsteffen commented Aug 31, 2022

  • ast::BindingMode is deleted and replaced with hir::BindingAnnotation (which is moved to ast)
  • BindingAnnotation is changed from an enum to a tuple struct e.g. BindingAnnotation(ByRef::No, Mutability::Mut)
  • Associated constants added for convenience BindingAnnotation::{NONE, REF, MUT, REF_MUT}

One goal is to make it more clear that BindingAnnotation merely represents syntax ref mut and not the actual binding mode. This was especially confusing since we had ast::BindingMode->hir::BindingAnnotation->thir::BindingMode.

I wish there were more symmetry between ByRef and Mutability (variant) naming (maybe Mutable::Yes?), and I also don't love how long the name BindingAnnotation is, but this seems like the best compromise. Ideas welcome.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 31, 2022
@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot
Copy link
Collaborator

rustbot commented Aug 31, 2022

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 31, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@camsteffen camsteffen force-pushed the refactor-binding-annotations branch from a597649 to aaf9b68 Compare August 31, 2022 15:18
@rust-log-analyzer

This comment has been minimized.

@camsteffen camsteffen force-pushed the refactor-binding-annotations branch from aaf9b68 to 097ca4d Compare August 31, 2022 16:26
@bors
Copy link
Contributor

bors commented Sep 1, 2022

☔ The latest upstream changes (presumably #101249) made this pull request unmergeable. Please resolve the merge conflicts.

@camsteffen camsteffen force-pushed the refactor-binding-annotations branch from 097ca4d to 9ea82d5 Compare September 2, 2022 18:03
@cjgillot
Copy link
Contributor

cjgillot commented Sep 4, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Sep 4, 2022

📌 Commit 9ea82d5 has been approved by cjgillot

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 Sep 4, 2022
@bors
Copy link
Contributor

bors commented Sep 5, 2022

⌛ Testing commit 9ea82d5 with merge b52a3be1828c81008d625765dc480609524da515...

@bors
Copy link
Contributor

bors commented Sep 5, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 5, 2022
@camsteffen
Copy link
Contributor Author

Looks quite spurious. @bors retry

@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 Sep 5, 2022
@rust-log-analyzer
Copy link
Collaborator

The job dist-aarch64-apple failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
      Memory: 14 GB
      System Firmware Version: VMW71.00V.13989454.B64.1906190538
      Apple ROM Info: [MS_VM_CERT/SHA1/27d66596a61c48dd3dc7216fd715126e33f59ae7]Welcome to the Virtual Machine
      SMC Version (system): 2.8f0
      Serial Number (system): VMD61DXbtTDd
      Provisioning UDID: 4203018E-580F-C1B5-9525-B745CECA79EB

hw.ncpu: 3
hw.byteorder: 1234

@bors
Copy link
Contributor

bors commented Sep 6, 2022

⌛ Testing commit 9ea82d5 with merge 6c358c6...

@bors
Copy link
Contributor

bors commented Sep 6, 2022

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 6c358c6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 6, 2022
@bors bors merged commit 6c358c6 into rust-lang:master Sep 6, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 6, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6c358c6): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.4% [1.2%, 1.6%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.9% [2.9%, 2.9%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean1 range count2
Regressions ❌
(primary)
1.5% [1.5%, 1.5%] 1
Regressions ❌
(secondary)
2.4% [2.1%, 2.7%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.5% [1.5%, 1.5%] 1

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@camsteffen camsteffen deleted the refactor-binding-annotations branch September 6, 2022 12:39
flip1995 pushed a commit to flip1995/rust that referenced this pull request Sep 9, 2022
…ons, r=cjgillot

`BindingAnnotation` refactor

* `ast::BindingMode` is deleted and replaced with `hir::BindingAnnotation` (which is moved to `ast`)
* `BindingAnnotation` is changed from an enum to a tuple struct e.g. `BindingAnnotation(ByRef::No, Mutability::Mut)`
* Associated constants added for convenience `BindingAnnotation::{NONE, REF, MUT, REF_MUT}`

One goal is to make it more clear that `BindingAnnotation` merely represents syntax `ref mut` and not the actual binding mode. This was especially confusing since we had `ast::BindingMode`->`hir::BindingAnnotation`->`thir::BindingMode`.

I wish there were more symmetry between `ByRef` and `Mutability` (variant) naming (maybe `Mutable::Yes`?), and I also don't love how long the name `BindingAnnotation` is, but this seems like the best compromise. Ideas welcome.
yvt added a commit to yvt/servo that referenced this pull request Oct 16, 2022
yvt added a commit to yvt/servo that referenced this pull request Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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
Development

Successfully merging this pull request may close these issues.

8 participants