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: VecDeques IntoIter fields to private #89053

Merged
merged 1 commit into from
Sep 19, 2021
Merged

refactor: VecDeques IntoIter fields to private #89053

merged 1 commit into from
Sep 19, 2021

Conversation

DeveloperC286
Copy link
Contributor

Made the fields of VecDeque's IntoIter private by creating a IntoIter::from(...) function to create a new instance of IntoIter and migrating usage to use IntoIter::from(...).

@rust-highfive
Copy link
Collaborator

r? @kennytm

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 17, 2021
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

Is there a motivation for this change? It looks like it's safe currently to construct the iterator from any VecDeque and there's no internal state otherwise, so the added constructor doesn't seem necessary?

library/alloc/src/collections/vec_deque/into_iter.rs Outdated Show resolved Hide resolved
@DeveloperC286
Copy link
Contributor Author

Is there a motivation for this change? It looks like it's safe currently to construct the iterator from any VecDeque and there's no internal state otherwise, so the added constructor doesn't seem necessary?

Hey, thank you for your quick feedback. My motivation behind this change and a few others I will be submitting, is I was told having fields of a struct accessible directly was a code smell. While no code is currently directly accessing the field this removes the possibility.

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

Alright, seems OK. I don't think this was necessarily a code smell or otherwise bad previously, since it was in practice just a crate-local field -- that's usually pretty harmless, especially when there's no invariants attached to the field (like here). But it is a little nicer if you can be confident that no other module is touching the field, so I'll approve this PR :)

@bors
Copy link
Contributor

bors commented Sep 17, 2021

📌 Commit 05b01cd has been approved by Mark-Simulacrum

@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 17, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Sep 19, 2021
…rivate, r=Mark-Simulacrum

refactor: VecDeques IntoIter fields to private

Made the fields of VecDeque's IntoIter private by creating a IntoIter::from(...) function to create a new instance of IntoIter and migrating usage to use IntoIter::from(...).
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2021
Rollup of 10 pull requests

Successful merges:

 - rust-lang#87960 (Suggest replacing an inexisting field for an unmentioned field)
 - rust-lang#88855 (Allow simd_shuffle to accept vectors of any length)
 - rust-lang#88966 (Check for shadowing issues involving block labels)
 - rust-lang#88996 (Fix linting when trailing macro expands to a trailing semi)
 - rust-lang#89017 (fix potential race in AtomicU64 time monotonizer)
 - rust-lang#89021 (Add a separate error for `dyn Trait` in `const fn`)
 - rust-lang#89051 (Add intra-doc links and small changes to `std::os` to be more consistent)
 - rust-lang#89053 (refactor: VecDeques IntoIter fields to private)
 - rust-lang#89055 (Suggest better place to add call parentheses for method expressions wrapped in parentheses)
 - rust-lang#89081 (Fix a typo)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e4dbe27 into rust-lang:master Sep 19, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 19, 2021
@DeveloperC286 DeveloperC286 deleted the into_iter_fields_to_private branch September 19, 2021 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants