Skip to content

Make clang::Cursor::fallible_semantic_parent make ffi call #172

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 8, 2016
Merged

Make clang::Cursor::fallible_semantic_parent make ffi call #172

merged 1 commit into from
Nov 8, 2016

Conversation

malisas
Copy link
Contributor

@malisas malisas commented Oct 30, 2016 •
edited
Loading

This PR fixes #120 . clang::Cursor::semantic_parent now just calls clang::Cursor::fallible_semantic_parent, and the ffi call has been moved into fallible_semantic_parent.

This change broke a number of tests which call is_toplevel() (which itself calls semantic_parent()):

panicked at called Option::unwrap() on a None value

So I re-wrote is_toplevel() to call fallible_semantic_parent(), which returns an Option<Cursor> type instead.

@highfive
Copy link

warning Warning warning

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

@malisas
Copy link
Contributor Author

malisas commented Oct 30, 2016

r? @fitzgen

@emilio
Copy link
Contributor

emilio commented Oct 31, 2016

This is conceptually ok, but breaks the tests very badly. Most of the callees handled the invalid cursor themselves, so they need to be fixed also.

@fitzgen
Copy link
Member

fitzgen commented Oct 31, 2016

Changes themselves look good. Did you re-run the tests? As @emilio pointed out, they're failing now.

I would git grep 'semantic_parent(' to find all callers, then look at each one to determine whether or not it is handling invalid cursors. If it is, then make it call fallible_semantic_parent and change its invalid cursor handling to when it gets a None. If it is not handling invalid cursors, then it is OK to call semantic_parent and indirectly do the unwrap().

Does that make sense?

@malisas
Copy link
Contributor Author

malisas commented Oct 31, 2016

Yes, thanks for the feedback! Did not think this was going to break things so I did not run the tests, oops.

@bors-servo
Copy link

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

@malisas malisas force-pushed the malisa-semantic-parent-call branch from fe01673 to bfc0290 Compare November 7, 2016 00:18
malisas
malisas commented Nov 7, 2016
View reviewed changes
src/clang.rs
// Yes, the second can happen with, e.g., macro definitions.
semantic_parent == tu || semantic_parent == tu.semantic_parent()
// Yes, this can happen with, e.g., macro definitions.
semantic_parent == tu.fallible_semantic_parent()
Copy link
Contributor Author

@malisas malisas Nov 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

At this point, semantic_parent.unwrap() is now None. Therefore true can only be returned if tu is None or tu.fallible_semantic_parent() is None. tu will never be None because it is a Cursor type, so the first check is now unnecessary.

@malisas
Copy link
Contributor Author

malisas commented Nov 7, 2016

I had to make changes to is_toplevel(). Tests should be passing now, but I'm a little concerned that I don't completely understand the purpose/context of this function (it appears to perform a similar task as the if sp == *self check in fallible_semantic_parent())....

Thank you to @fitzgen for helping me figure out what's going on!

fitzgen
fitzgen approved these changes Nov 7, 2016
View reviewed changes
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @malisas!

@fitzgen
Copy link
Member

fitzgen commented Nov 7, 2016

r? @emilio -- care to rubber stamp this?

@highfive highfive assigned emilio and unassigned fitzgen Nov 7, 2016
@emilio
Copy link
Contributor

emilio commented Nov 7, 2016

Oh, sure, sorry I missed this one. @bors-servo r=fitzgen,emilio

@bors-servo
Copy link

📌 Commit bfc0290 has been approved by fitzgen,emilio

@highfive highfive assigned fitzgen and unassigned emilio Nov 7, 2016
@bors-servo
Copy link

⌛ Testing commit bfc0290 with merge 86c3bba...

bors-servo pushed a commit that referenced this pull request Nov 7, 2016
Auto merge of #172 - malisas:malisa-semantic-parent-call, r=fitzgen,e…
86c3bba 
…milio

Make clang::Cursor::fallible_semantic_parent make ffi call

This PR fixes #120 . `clang::Cursor::semantic_parent` now just calls `clang::Cursor::fallible_semantic_parent`, and the ffi call has been moved into `fallible_semantic_parent`.

This change broke a number of tests which call `is_toplevel()` (which itself calls `semantic_parent()`):
> panicked at called Option::unwrap() on a None value

So I re-wrote `is_toplevel()` to call `fallible_semantic_parent()`, which returns an `Option<Cursor>` type instead.
@bors-servo
Copy link

☀️ Test successful - status-travis

@bors-servo bors-servo merged commit bfc0290 into rust-lang:master Nov 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fitzgen fitzgen fitzgen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

clang::Cursor::semantic_parent should call clang::Cursor::fallible_semantic_parent -- not the other way around
5 participants
@malisas @highfive @emilio @fitzgen @bors-servo