Skip to content

BindingAnnotation refactor #101241

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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
Contributor

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
Collaborator

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
Collaborator

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
Collaborator

bors commented Sep 5, 2022

⌛ Testing commit 9ea82d5 with merge b52a3be1828c81008d625765dc480609524da515...

@bors
Copy link
Collaborator

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
Collaborator

bors commented Sep 6, 2022

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

@bors
Copy link
Collaborator

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
# for free to join this conversation on GitHub. Already have an account? # 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