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

143 comment get tag attr #158

Merged
merged 1 commit into from Oct 31, 2016
Merged

143 comment get tag attr #158

merged 1 commit into from Oct 31, 2016

Conversation

ajnirp
Copy link

@ajnirp ajnirp commented Oct 29, 2016

Fixes #143

@highfive
Copy link

warning Warning warning

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

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.

Looks good to me with those changes, thanks!

@@ -134,20 +134,23 @@ impl Annotations {
if comment.kind() == CXComment_HTMLStartTag &&
comment.get_tag_name() == "div" &&
comment.get_num_tag_attrs() > 1 &&
comment.get_tag_attr_name(0) == "rustbindgen" {
comment.get_tag_attr_name(0) == Some("rustbindgen".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.

comment.get_tag_attr_name(0).as_ref() == Some("rustbindgen")

Copy link
Author

@ajnirp ajnirp Oct 30, 2016

Choose a reason for hiding this comment

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

Good catch, thank you! I had to use as_ref().map(|s| s.as_str()) to prevent the compiler from complaining about seeing &String and expecting &'static str.

=> self.accessor_kind = Some(parse_accessor(&value)),
_ => {},
let name_opt = comment.get_tag_attr_name(i);
match name_opt {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use match comment.get_tag_attr_name(i).unwrap().as_str(), since we know we're inside the bounds.

Copy link
Author

Choose a reason for hiding this comment

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

Done!

@emilio
Copy link
Contributor

emilio commented Oct 29, 2016

Also, you seem to have two commits with the same message, feel free to squash them if you wish, thanks!

@emilio
Copy link
Contributor

emilio commented Oct 30, 2016

@bors-servo r+

Thanks!

@bors-servo
Copy link

📌 Commit 6298736 has been approved by emilio

@bors-servo
Copy link

⌛ Testing commit 6298736 with merge 9db4d1f...

bors-servo pushed a commit that referenced this pull request Oct 30, 2016
@bors-servo
Copy link

💔 Test failed - status-travis

unsafe {
String_ { x: clang_HTMLStartTag_getAttrValue(self.x, idx) }.to_string()
pub fn get_tag_attr_value(&self, idx: c_uint) -> Option<String> {
if idx < self.get_num_tag_attrs() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong, the check should be in the other direction (if idx < num { Some(..) }). Same everywhere else.

@emilio
Copy link
Contributor

emilio commented Oct 31, 2016

@bors-servo r+

Thanks again :)

@bors-servo
Copy link

📌 Commit 11f0839 has been approved by emilio

@bors-servo
Copy link

⚡ Test exempted - status

@bors-servo bors-servo merged commit 11f0839 into rust-lang:master Oct 31, 2016
bors-servo pushed a commit that referenced this pull request Oct 31, 2016
@ajnirp
Copy link
Author

ajnirp commented Oct 31, 2016

Thank you!

@ajnirp ajnirp deleted the 143-comment-get-tag-attr branch October 31, 2016 20:42
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.

None yet

4 participants