Skip to content

Use iterators for comment children and attributes fix #166 #169

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 2 commits into from
Oct 31, 2016

Conversation

jeanphilippeD
Copy link
Contributor

@jeanphilippeD jeanphilippeD commented Oct 30, 2016

Fix #166

@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@jeanphilippeD jeanphilippeD changed the title Use iterators for comment children partial fix #166 Use iterators for comment children and attributes fix #166 Oct 30, 2016
@jeanphilippeD jeanphilippeD force-pushed the issue166 branch 2 times, most recently from 6a4ddeb to 9e8bae7 Compare October 30, 2016 05:52
Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

The rest looks awesome, thanks for doing this!

You'll have to rebase over #158 I think (I didn't expect you to do it this fast!), but apart from that and the nits this is great, thanks!

unsafe {
String_ { x: clang_HTMLStartTag_getAttrName(self.x, idx) }.to_string()
/// An iterator for a comment's children
pub struct CommentChildrenIntoIterator {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Probably CommentChildrenIterator? IntoIterator usually means that it "consumes" the value, which doesn't make a lot of sense in this case.

}

/// An iterator for a comment's attributes
pub struct CommentAttributesIntoIterator {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, CommentAttributesIterator seems like a better name for this, since it doesn't consume the comment.

name: String_ { x: unsafe { clang_HTMLStartTag_getAttrName(self.x, idx) } }.to_string(),
value: String_ { x: unsafe { clang_HTMLStartTag_getAttrValue(self.x, idx) } }.to_string()
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Rust style is } else {.

@jeanphilippeD
Copy link
Contributor Author

You'll have to rebase over #158 I think (I didn't expect you to do it this fast!), but apart from that and the nits this is great, thanks!

I'm not sure what to do here.
I believe #158 is not merged yet, there seem to have been a merge failure.

@jeanphilippeD
Copy link
Contributor Author

Thanks for the review.
I fixed the review comment. I'll wait until the other PR is merged to master to rebase this one.

@bors-servo
Copy link

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

@emilio
Copy link
Contributor

emilio commented Oct 31, 2016

@bors-servo r+

Neat, thanks!

@bors-servo
Copy link

📌 Commit 91eb86d has been approved by emilio

@bors-servo
Copy link

⌛ Testing commit 91eb86d with merge a960fb8...

bors-servo pushed a commit that referenced this pull request Oct 31, 2016
Use iterators for comment children and attributes fix #166

Fix #166
@bors-servo
Copy link

☀️ Test successful - status-travis

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.

4 participants