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

Feature gate enums in offset_of #117537

Merged
merged 4 commits into from
Nov 5, 2023
Merged

Conversation

GKFX
Copy link
Contributor

@GKFX GKFX commented Nov 3, 2023

As requested at #106655 (comment), put enums in offset_of behind their own feature gate.

@rustbot label F-offset_of

@rustbot
Copy link
Collaborator

rustbot commented Nov 3, 2023

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@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. F-offset_of `#![feature(offset_of)]` labels Nov 3, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@deltragon
Copy link
Contributor

Would it make sense to also gate everything but the simplest form, like nested fields? The syntax of nested fields may interact with the syntax for enums, and can be emulated by doing multiple calls to offset_off.

@GKFX
Copy link
Contributor Author

GKFX commented Nov 3, 2023

Would it make sense to also gate everything but the simplest form, like nested fields? The syntax of nested fields may interact with the syntax for enums, and can be emulated by doing multiple calls to offset_off.

Syntax is already listed as a concern on the FCP; I think it should be resolved before any form of offset_of is stabilized, rather than letting the issue linger and stabilising only a very basic offset_of.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

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

The implementation looks good. We can still decide to stabilize both at the same time further down the road, if we want to.

@est31 est31 mentioned this pull request Nov 4, 2023
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

r=me with formatting fixed

@cjgillot cjgillot 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 Nov 4, 2023
@GKFX
Copy link
Contributor Author

GKFX commented Nov 4, 2023

@cjgillot Could you clarify what formatting changes you want? The CI passes and there doesn't seem to be anything too ugly in the diff.

Co-authored-by: Camille Gillot <gillot.camille@gmail.com>
@GKFX
Copy link
Contributor Author

GKFX commented Nov 4, 2023

@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 Nov 4, 2023
@cjgillot
Copy link
Contributor

cjgillot commented Nov 5, 2023

Thanks.
@bors r+

@bors
Copy link
Contributor

bors commented Nov 5, 2023

📌 Commit 00a9ed3 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 Nov 5, 2023
@bors
Copy link
Contributor

bors commented Nov 5, 2023

⌛ Testing commit 00a9ed3 with merge 992943d...

@bors
Copy link
Contributor

bors commented Nov 5, 2023

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 992943d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 5, 2023
@bors bors merged commit 992943d into rust-lang:master Nov 5, 2023
12 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Nov 5, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (992943d): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

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.

mean range count
Regressions ❌
(primary)
0.6% [0.5%, 0.6%] 3
Regressions ❌
(secondary)
1.0% [1.0%, 1.0%] 1
Improvements ✅
(primary)
-0.9% [-1.4%, -0.6%] 3
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.2% [-1.4%, 0.6%] 6

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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.5% [-0.7%, -0.4%] 11
Improvements ✅
(secondary)
-1.1% [-1.1%, -1.1%] 1
All ❌✅ (primary) -0.5% [-0.7%, -0.4%] 11

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 638.392s -> 635.747s (-0.41%)
Artifact size: 304.54 MiB -> 304.50 MiB (-0.01%)

bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Nov 6, 2023
84: Automated pull from upstream `master` r=Dajamante a=github-actions[bot]


This PR pulls the following changes from the upstream repository:

* rust-lang/rust#117585
* rust-lang/rust#117576
* rust-lang/rust#96979
* rust-lang/rust#117191
* rust-lang/rust#117179
* rust-lang/rust#117574
* rust-lang/rust#117537
* rust-lang/rust#117608
  * rust-lang/rust#117596
  * rust-lang/rust#117588
  * rust-lang/rust#117524
  * rust-lang/rust#116017
* rust-lang/rust#117504
* rust-lang/rust#117469
* rust-lang/rust#116218
* rust-lang/rust#117589
* rust-lang/rust#117581
* rust-lang/rust#117503
* rust-lang/rust#117590
  * rust-lang/rust#117583
  * rust-lang/rust#117570
  * rust-lang/rust#117562
  * rust-lang/rust#117534
  * rust-lang/rust#116894
  * rust-lang/rust#110340
* rust-lang/rust#113343
* rust-lang/rust#117579
* rust-lang/rust#117094
* rust-lang/rust#117566
* rust-lang/rust#117564
  * rust-lang/rust#117554
  * rust-lang/rust#117550
  * rust-lang/rust#117343
* rust-lang/rust#115274
* rust-lang/rust#117540
* rust-lang/rust#116412
* rust-lang/rust#115333
* rust-lang/rust#117507
* rust-lang/rust#117538
  * rust-lang/rust#117533
  * rust-lang/rust#117523
  * rust-lang/rust#117520
  * rust-lang/rust#117505
  * rust-lang/rust#117434
* rust-lang/rust#117535
* rust-lang/rust#117510
* rust-lang/rust#116439
* rust-lang/rust#117508



Co-authored-by: Ben Wiederhake <BenWiederhake.GitHub@gmx.de>
Co-authored-by: SabrinaJewson <sejewson@gmail.com>
Co-authored-by: J-ZhengLi <lizheng135@huawei.com>
Co-authored-by: koka <koka.code@gmail.com>
Co-authored-by: bjorn3 <17426603+bjorn3@users.noreply.github.com>
Co-authored-by: Joshua Liebow-Feeser <joshlf@users.noreply.github.com>
Co-authored-by: lengyijun <sjtu5140809011@gmail.com>
Co-authored-by: Zalathar <Zalathar@users.noreply.github.com>
Co-authored-by: Oli Scherer <git-spam-no-reply9815368754983@oli-obk.de>
Co-authored-by: Philipp Krones <hello@philkrones.com>
Co-authored-by: y21 <30553356+y21@users.noreply.github.com>
Co-authored-by: bors <bors@rust-lang.org>
Co-authored-by: bohan <bohan-zhang@foxmail.com>
@GKFX GKFX deleted the offset-of-enum-feature branch January 2, 2024 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-offset_of `#![feature(offset_of)]` 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