refactor: removing library/alloc/src/vec/mod.rs ignore-tidy-filelength #78934

DeveloperC286 · 2020-11-11T00:01:58Z

This PR removes the need for ignore-tidy-filelength for library/alloc/src/vec/mod.rs which is part of the issue #60302

It is probably easiest to review this PR by looking at it commit by commit rather than looking at the overall diff.

rust-highfive · 2020-11-11T00:02:01Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

bors · 2020-11-11T04:36:47Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

m-ou-se

Thanks for working on this!

I have verified everything up to 643b7d8 just moves things unmodified, except for a few pubs and some formatting changes.

I have a few comments:

m-ou-se · 2020-11-16T12:34:40Z

library/alloc/src/vec/mod.rs

-macro_rules! __impl_slice_eq1 {
-    ([$($vars:tt)*] $lhs:ty, $rhs:ty $(where $ty:ty: $bound:ident)?, #[$stability:meta]) => {
-        #[$stability]
-        impl<A, B, $($vars)*> PartialEq<$rhs> for $lhs
-        where
-            A: PartialEq<B>,
-            $($ty: $bound)?
-        {
-            #[inline]
-            fn eq(&self, other: &$rhs) -> bool { self[..] == other[..] }
-            #[inline]
-            fn ne(&self, other: &$rhs) -> bool { self[..] != other[..] }
-        }
-    }
-}
-
 __impl_slice_eq1! { [] Vec<A>, Vec<B>, #[stable(feature = "rust1", since = "1.0.0")] }


I think it'd be good to keep macros like this and their list of invocations together.

Maybe instead of a macros.rs, there could be a slice_eq.rs or something like that.

m-ou-se · 2020-11-16T12:40:05Z

library/alloc/src/vec/drain_filter.rs

+    where
+        F: FnMut(&mut T) -> bool,
+{
+    pub(crate) vec: &'a mut Vec<T>,


@KodrAus said on zulip:

Hmm, I feel like PRs that are splitting up large modules into smaller child ones that use mostly pub(super) or pub(crate) internals could be missing an opportunity to define better privacy boundaries 🤔

It'd be good to see if we can avoid making these all pub(crate). pub(super) would already be a bit better, but it'd be good to check if we can make these private, possibly by adding a pub(super) function or two, depending on what the parent module actually needs.

If that's a lot of work, it would be okay to just merge this with pub(super) to avoid bitrot/merge conflicts, and fix it up in a smaller follow-up PR.

m-ou-se · 2020-11-16T12:50:52Z

library/alloc/src/vec/clone_on_write.rs

@@ -0,0 +1,34 @@
+use crate::borrow::Cow;


It might be good to just name this file cow.rs. I think the word Cow is more recognizable in Rust than clone_on_write.

TimDiekmann · 2020-11-16T14:31:19Z

It would be good if we could wait to merge the PR until #78461 has landed, as many things of Vec has changed there.

DeveloperC286 · 2020-11-16T17:12:16Z

Hi @m-ou-se thanks for your review and the comments 👍 I will take your comments onboard and fix up my PR. I will just wait until @TimDiekmann 's #78461 has been merged. @TimDiekmann any idea on the timeline or other PR's I should wait on?

TimDiekmann · 2020-11-16T17:19:20Z

Thanks for your consideration!
My pull request is still waiting for a review but the crater run was successful so this shouldn't take too long as soon as @Amanieu has time to look into it.

I don't think you have to be considerate of other PRs. Most likely AllocRef and Vec::alloc_ref will be renamed to Allocator and Vec::allocator respectively, but these are only small changes that I can easily rebase later.

bors · 2020-11-22T01:32:48Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

DeveloperC286 · 2020-12-05T15:04:36Z

@m-ou-se Hey! Sorry it took me a while to get this PR updated.

I followed your suggestion and used cow.rs instead of clone_on_write.rs
I removed the marcos.rs and instead the macros now live with the structs/traits that use them.
Also used pub (super) everywhere I could instead of pub (crate), making it private seemed to require a decent amount of changes so maybe that might need to be a separate issue/PR?

Thanks,

bors · 2020-12-15T11:46:21Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

DeveloperC286 · 2020-12-18T00:45:15Z

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

bors · 2020-12-29T03:15:39Z

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

m-ou-se

Thanks a lot for working on this! And sorry for the delay.

This looks great! I have one tiny nit:

m-ou-se · 2020-12-30T15:32:49Z

library/alloc/src/vec/into_iter.rs

+/// This `struct` is created by the `into_iter` method on [`super::Vec`] (provided
+/// by the [`IntoIterator`] trait).


This link used to be [`Vec`]. Let's keep the visible text the same:

Suggested change

/// This `struct` is created by the `into_iter` method on [`super::Vec`] (provided

/// by the [`IntoIterator`] trait).

/// This `struct` is created by the `into_iter` method on [`Vec`](super::Vec)

/// (provided by the [`IntoIterator`] trait).

m-ou-se · 2020-12-30T15:35:15Z

I'll just commit my suggestion and approve your PR, to not delay it any further. Hope you don't mind. :)

@bors r+

bors · 2020-12-30T15:35:17Z

📌 Commit 16834a8 has been approved by m-ou-se

DeveloperC286 · 2020-12-30T15:51:38Z

I'll just commit my suggestion and approve your PR, to not delay it any further. Hope you don't mind. :)

@bors r+

Thank you for your review and for fixing the documentation link for me! 👍

Rollup of 9 pull requests Successful merges: - rust-lang#78934 (refactor: removing library/alloc/src/vec/mod.rs ignore-tidy-filelength) - rust-lang#79479 (Add `Iterator::intersperse`) - rust-lang#80128 (Edit rustc_ast::ast::FieldPat docs) - rust-lang#80424 (Don't give an error when creating a file for the first time) - rust-lang#80458 (Some Promotion Refactoring) - rust-lang#80488 (Do not create dangling &T in Weak<T>::drop) - rust-lang#80491 (Miri: make size/align_of_val work for dangling raw ptrs) - rust-lang#80495 (Rename kw::Invalid -> kw::Empty) - rust-lang#80513 (Add regression test for rust-lang#80062) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

…ec, r=m-ou-se" This reverts commit 242a252, reversing changes made to 507bff9.

rylev · 2021-01-08T16:17:34Z

@DeveloperC286 @m-ou-se this seems to be the cause of a slight perf regression as you can see in #80814

I assume this is likely a change in inlining which often happens when moving things around. We should look into if we can gain some of that perf lost back.

rust-highfive assigned joshtriplett Nov 11, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 11, 2020

jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 16, 2020

m-ou-se assigned m-ou-se and unassigned joshtriplett Nov 16, 2020

m-ou-se reviewed Nov 16, 2020

View reviewed changes

m-ou-se 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 28, 2020

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 Dec 18, 2020

DeveloperC286 added 9 commits December 29, 2020 14:03

refactor: moving vec.rs to vec/mod.rs

5182776

refactor: moving DrainFilter into drain_filter.rs

434e5d1

refactor: moving Splice into splice.rs

17593f2

refactor: moving Drain into drain.rs

6bf9608

refactor: moved Vec impl Cow into cow.rs

2580822

refactor: moved IntoIter into into_iter.rs

9361390

refactor: moving AsIntoIter into into_iter.rs

2a12489

refactor: moved IsZero into is_zero.rs

840c4e2

refactor: moving SourceIterMarker into source_iter_marker.rs

5ac6709

DeveloperC286 added 12 commits December 29, 2020 14:03

refactor: moved PartialEq into partial_eq

dc46013

refactor: moved SpecFromElem to spec_from_elem.rs

a2f4bc0

refactor: moved SetLenOnDrop to set_len_on_drop

a3f3fc5

refactor: moved InPlaceDrop into in_place_drop.rs

9e08ce7

refactor: moved SpecFromIterNested to spec_from_iter_nested.rs

56d82b3

refactor: moving SpecFromIter into spec_from_iter.rs

d24a277

refactor: moved SpecExtend into spec_extend.rs

bd49a60

refactor: removing // ignore-tidy-filelength

6002b28

style: applying Rust style

2de8356

fix: moved import into #[cfg(test)]

80f10d7

docs: fixing references

f7a6f0c

test: updated expected Vec src path

206b2ca

m-ou-se reviewed Dec 30, 2020

View reviewed changes

Fix rustdoc link in vec/into_iter.rs.

16834a8

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 Dec 30, 2020

m-ou-se mentioned this pull request Dec 30, 2020

Rollup of 9 pull requests #80530

Merged

bors merged commit 242a252 into rust-lang:master Dec 31, 2020

rustbot added this to the 1.51.0 milestone Dec 31, 2020

DeveloperC286 deleted the issue_60302_vec branch December 31, 2020 00:37

rylev added a commit to rylev/rust that referenced this pull request Jan 8, 2021

Revert "Rollup merge of rust-lang#78934 - DeveloperC286:issue_60302_v…

e816079

…ec, r=m-ou-se" This reverts commit 242a252, reversing changes made to 507bff9.

rylev mentioned this pull request Jan 8, 2021

Revert "Rollup merge of #78934 - DeveloperC286:issue_60302_vec, r=m-ou-se" #80814

Closed

danii mentioned this pull request Mar 10, 2021

Split up files with // ignore-tidy-filelength #60302

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: removing library/alloc/src/vec/mod.rs ignore-tidy-filelength #78934

refactor: removing library/alloc/src/vec/mod.rs ignore-tidy-filelength #78934

DeveloperC286 commented Nov 11, 2020

rust-highfive commented Nov 11, 2020

bors commented Nov 11, 2020

m-ou-se left a comment

m-ou-se Nov 16, 2020

m-ou-se Nov 16, 2020

m-ou-se Nov 16, 2020

TimDiekmann commented Nov 16, 2020

DeveloperC286 commented Nov 16, 2020

TimDiekmann commented Nov 16, 2020 •

edited

Loading

bors commented Nov 22, 2020

DeveloperC286 commented Dec 5, 2020

bors commented Dec 15, 2020

DeveloperC286 commented Dec 18, 2020

bors commented Dec 29, 2020

m-ou-se left a comment

m-ou-se Dec 30, 2020

m-ou-se commented Dec 30, 2020

bors commented Dec 30, 2020

DeveloperC286 commented Dec 30, 2020

rylev commented Jan 8, 2021

		/// This `struct` is created by the `into_iter` method on [`super::Vec`] (provided
		/// by the [`IntoIterator`] trait).

refactor: removing library/alloc/src/vec/mod.rs ignore-tidy-filelength #78934

refactor: removing library/alloc/src/vec/mod.rs ignore-tidy-filelength #78934

Conversation

DeveloperC286 commented Nov 11, 2020

rust-highfive commented Nov 11, 2020

bors commented Nov 11, 2020

m-ou-se left a comment

Choose a reason for hiding this comment

m-ou-se Nov 16, 2020

Choose a reason for hiding this comment

m-ou-se Nov 16, 2020

Choose a reason for hiding this comment

m-ou-se Nov 16, 2020

Choose a reason for hiding this comment

TimDiekmann commented Nov 16, 2020

DeveloperC286 commented Nov 16, 2020

TimDiekmann commented Nov 16, 2020 • edited Loading

bors commented Nov 22, 2020

DeveloperC286 commented Dec 5, 2020

bors commented Dec 15, 2020

DeveloperC286 commented Dec 18, 2020

bors commented Dec 29, 2020

m-ou-se left a comment

Choose a reason for hiding this comment

m-ou-se Dec 30, 2020

Choose a reason for hiding this comment

m-ou-se commented Dec 30, 2020

bors commented Dec 30, 2020

DeveloperC286 commented Dec 30, 2020

rylev commented Jan 8, 2021

TimDiekmann commented Nov 16, 2020 •

edited

Loading