Skip to content

Priv fields #9969

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

Merged
merged 7 commits into from
Oct 22, 2013
Merged

Priv fields #9969

merged 7 commits into from
Oct 22, 2013

Conversation

reedlepee123
Copy link
Contributor

No description provided.

@@ -50,6 +50,7 @@ use std::borrow;

/// As sync::condvar, a mechanism for unlock-and-descheduling and signaling.
pub struct Condvar<'self> {
// all were already priv
Copy link
Member

Choose a reason for hiding this comment

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

None of these comments are necessary since we (the reviewers) can see exactly what you changed from the diff (this view).

Can you remove them please?

@huonw
Copy link
Member

huonw commented Oct 20, 2013

Thank you for this, it looks pretty good!

I'd like the removal of the comments like // all already priv, since this information is already accessible from the git history.

Also, this needs to be rebased on top of the master branch of the mozilla/rust repo.

query: Query,
fragment: Option<~str>
// all were made privv bt reedlepee
priv scheme: ~str,
Copy link
Member

Choose a reason for hiding this comment

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

I have a feeling that these are designed for use outside this module (but I'm not sure).

If they are, then the should still be public.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I just checked in servo, and it does use the fields directly, so these need to be public. (git checkout src/libextra/url.rs should work.)

@reedlepee123
Copy link
Contributor Author

I removed unnecessary comments and white spaces and changed some fields to public as suggested in above comments .

data: &'self mut T,
token: sync::RWLockWriteMode<'self>,
poison: PoisonOnFail,

Copy link
Member

Choose a reason for hiding this comment

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

Extra empty line here.

@@ -38,7 +38,7 @@ pub struct FormatSpec {
fill: char,
align: parse::Alignment,
flags: uint,
precision: Count,
precision: Count,
Copy link
Member

Choose a reason for hiding this comment

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

Extra space.

@huonw
Copy link
Member

huonw commented Oct 20, 2013

It's almost ready to go! :) Although it still needs to be rebased.

(cc @catamorphism.)

@bstrie
Copy link
Contributor

bstrie commented Oct 20, 2013

@reedlepee123 You'll need to rebase and fix the merge conflicts before we can approve this. Find either myself or tjc on IRC if you would like some instruction on how to do this.

@bstrie
Copy link
Contributor

bstrie commented Oct 22, 2013

This closes #4386.

@bors bors closed this Oct 22, 2013
@bors bors merged commit 7e6f5bb into rust-lang:master Oct 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants