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

Refactor alloc/rc.rs into separate files #81272

Closed
wants to merge 1 commit into from
Closed

Refactor alloc/rc.rs into separate files #81272

wants to merge 1 commit into from

Conversation

yoshuawuyts
Copy link
Member

Similar to #81269, this patch splits alloc/rc.rs into a submodule, tracking Rc and Weak in separate files making it easier to work on the individual types. Thanks!

Changes

  • alloc::rc::Rc now lives in libraries/alloc/rc/rc.rs
  • alloc::rc::Weak now lives in libraries/alloc/rc/weak.rs
  • the majority of tests now live in alloc/tests/arc.rs
  • one test which tests internals is in alloc/rc/rc.rs

r? @KodrAus @Mark-Simulacrum

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

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@yoshuawuyts
Copy link
Member Author

Fixed the merge conflict.

If CI passes, I would appreciate a review on this. This is a rather large refactor, and keeping it up to date week over week can be challenging.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@KodrAus
Copy link
Contributor

KodrAus commented Feb 10, 2021

Thanks for working on this @yoshuawuyts!

It looks like we've got a tidy lint failure, but I'll run through the diff in the meantime.

@KodrAus
Copy link
Contributor

KodrAus commented Feb 10, 2021

I manually mapped the diff back into the original rc.rs file and everything looks like it found its way into the new structure. I'll check out the tidy error and try get this through.

@KodrAus
Copy link
Contributor

KodrAus commented Feb 10, 2021

@bors r+

Optimistically. I'll check back in here if the build fails.

@bors
Copy link
Contributor

bors commented Feb 10, 2021

📌 Commit 25c79f6e1633b4f5c32897e738cf59d9bffe3383 has been approved by KodrAus

@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 Feb 10, 2021
@Mark-Simulacrum
Copy link
Member

@bors rollup=never

These are frequently perf sensitive, unfortunately.

@bors
Copy link
Contributor

bors commented Feb 10, 2021

⌛ Testing commit 25c79f6e1633b4f5c32897e738cf59d9bffe3383 with merge ffd1d01aba565e49ff9fea164d09310258468286...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 10, 2021

💔 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 Feb 10, 2021
@KodrAus
Copy link
Contributor

KodrAus commented Feb 10, 2021

@bors try

@bors
Copy link
Contributor

bors commented Feb 10, 2021

⌛ Trying commit f827af42634db2e00b37c6f5f26f662c14787b76 with merge 21fef35f54cf849c200204fae70a193547de1b93...

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 10, 2021

💔 Test failed - checks-actions

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 10, 2021
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 10, 2021
@bors
Copy link
Contributor

bors commented Feb 13, 2021

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

@KodrAus
Copy link
Contributor

KodrAus commented Feb 21, 2021

It looks like we've run into a few more merge conflicts. Sorry for the pain here @yoshuawuyts! Maybe what we could do here is either:

  • Pick a time when we're both active and try shepherd this through together quickly
  • Try splitting it up in phases, where each file gets its own PR to reduce the chance of conflicts appearing

@yoshuawuyts
Copy link
Member Author

@KodrAus picking a time would have my preference. Let's chat on Zulip?

- `alloc::rc::Rc` now lives in `libraries/alloc/rc/rc.rs`
- `alloc::rc::Weak` now lives in `libraries/alloc/rc/weak.rs`
-  the majority of tests now live in `alloc/tests/arc.rs`
-  one test which tests internals is in `alloc/rc/rc.rs`

Other changes:

- Remove trailing newline
- Remove unused re-exports
- Fix rc re-exports

Co-Authored-By: Ashley Mannix <kodraus@hey.com>
@KodrAus
Copy link
Contributor

KodrAus commented Feb 24, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Feb 24, 2021

📌 Commit 65bbb3d has been approved by KodrAus

@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 Feb 24, 2021
@bors
Copy link
Contributor

bors commented Feb 24, 2021

⌛ Testing commit 65bbb3d with merge 661c87708b69bd03ed3a1a31adbcba2c9032fbb4...

@rust-log-analyzer
Copy link
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] build_script_build test:false 0.567
   Compiling rustc-std-workspace-core v1.99.0 (/checkout/library/rustc-std-workspace-core)
[RUSTC-TIMING] rustc_std_workspace_core test:false 0.025
   Compiling alloc v0.0.0 (/checkout/library/alloc)
error: unused imports: `MarkerEq`, `is_dangling`
   --> library/alloc/src/rc/mod.rs:257:24
    |
257 | pub(crate) use utils::{is_dangling, MarkerEq};
    |                        ^^^^^^^^^^^  ^^^^^^^^
    |
    = note: `-D unused-imports` implied by `-D warnings`
[RUSTC-TIMING] compiler_builtins test:false 1.781
error: aborting due to previous error

[RUSTC-TIMING] alloc test:false 1.274
[RUSTC-TIMING] alloc test:false 1.274
error: could not compile `alloc`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
[RUSTC-TIMING] core test:false 16.926
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "thumbv6m-none-eabi" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "-p" "alloc" "--manifest-path" "/checkout/library/alloc/Cargo.toml" "--features" "compiler-builtins-mem compiler-builtins-c" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap --stage 2 test --host= --target thumbv6m-none-eabi,thumbv7m-none-eabi,thumbv7em-none-eabi,thumbv7em-none-eabihf src/test/run-make
Build completed unsuccessfully in 0:14:10

@bors
Copy link
Contributor

bors commented Feb 24, 2021

💔 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 Feb 24, 2021
@Dylan-DPC-zz
Copy link

@bors r-

@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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 2, 2021
@crlf0710
Copy link
Member

@yoshuawuyts Ping from triage, it seems CI is still failing here.

@bors
Copy link
Contributor

bors commented Apr 1, 2021

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

@yoshuawuyts
Copy link
Member Author

More patches have been merged since. I'm going to shelve this for now (:

@yoshuawuyts yoshuawuyts closed this Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants