Skip to content

/cargo

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

Remove some cells from Config #4578

Merged
merged 3 commits into from Oct 7, 2017

Conversation

Reviewers

@alexcrichton alexcrichton

Assignees

@matklad matklad

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@derekdreery @rust-highfive @matklad @alexcrichton @bors
@derekdreery
Copy link
Contributor

derekdreery commented Oct 4, 2017

In most cases it makes sense to not use interior mutability in Config, so that mutability is more transparent. This PR removes Cells in these cases. It leaves those cases where parameters are only initialized on first access (LazyCell).

@rust-highfive rust-highfive assigned matklad Oct 4, 2017

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link

rust-highfive commented Oct 4, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (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.
@derekdreery

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

derekdreery commented Oct 4, 2017

The doc comments on frozen and locked could be improved, could anyone suggest what it should say?
@matklad

This comment has been minimized.

Sign in to view
Copy link
Member

matklad commented Oct 4, 2017

LGTM, but let's ask @alexcrichton as well!
@alexcrichton

alexcrichton reviewed Oct 4, 2017
View changes

Copy link
Member

alexcrichton left a comment

Looks great, thanks!
src/cargo/util/config.rs Outdated
frozen: Cell<bool>,
locked: Cell<bool>,
extra_verbose: bool,
/// TODO to do with Cargo.lock

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Oct 4, 2017

Member

This has to do with the --frozen flag being passed on the command line, namely we're not allowed to touch the network
src/cargo/util/config.rs Outdated
extra_verbose: bool,
/// TODO to do with Cargo.lock
frozen: bool,
/// TODO to do with Cargo.lock

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Oct 4, 2017

Member

This has to do with the --locked flag, namely we're not allowed to change lock files
@derekdreery

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

derekdreery commented Oct 6, 2017

Thanks I'll update
@derekdreery
Update docs (replace todos)
Loading status checks…
24d745a
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Oct 7, 2017

@bors: r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 7, 2017

📌 Commit 24d745a has been approved by alexcrichton
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 7, 2017

⌛️ Testing commit 24d745a with merge 446ed6e...

bors added a commit that referenced this pull request Oct 7, 2017

@bors
Auto merge of #4578 - derekdreery:refactor/cargo_util_config, r=alexc… 
…richton

Remove some cells from Config

In most cases it makes sense to not use interior mutability in Config, so that mutability is more transparent. This PR removes Cells in these cases. It leaves those cases where parameters are only initialized on first access (LazyCell).
Loading status checks…
446ed6e
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 7, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 446ed6e to master...

@bors bors merged commit 24d745a into rust-lang:master Oct 7, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.