Skip to content

clang::Cursor should never be the null cursor #119

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

Open
fitzgen opened this issue Oct 24, 2016 · 5 comments
Open

clang::Cursor should never be the null cursor #119

fitzgen opened this issue Oct 24, 2016 · 5 comments

Comments

@fitzgen
Copy link
Member

fitzgen commented Oct 24, 2016

Right now we have various methods on clang::Cursor that return Options just because (I think?) we call underlying clang functions that return null if the cursor is the "null cursor" (see clang_getNullCursor and clang_isNull).

It would be better if these methods did not return Option and always returned a value, if clang::Cursor was never the null cursor, and the place(s) where we first get a cursor return an Option<Cursor> to handle the null cursor case. If we adopt these invariants, we should also assert them throught clang::Cursor methods.

What do you think @emilio?

@emilio
Copy link
Contributor

emilio commented Nov 1, 2016

This sounds good, ideally we could try to never have a null/invalid cursor around, and we'd constrain their existence to the clang module. It's not a super-easy task though.

@ekse
Copy link
Contributor

ekse commented Aug 16, 2018

Is this change still wanted? It sounds like something that wouldn't be too difficult to do.

@emilio
Copy link
Contributor

emilio commented Aug 16, 2018

I think so, yeah.

bors-servo pushed a commit that referenced this issue Aug 18, 2018
Change a call to add_item that was passing a NullCursor.

As a first step for issue #119, I modified the only call where we created a NullCursor to pass a None instead.
@ekse
Copy link
Contributor

ekse commented Aug 18, 2018

I did some tests by adding debug_assert!(ret.is_valid()) at the end of methods that are returning a Cursor and running the test suite. The Cursor methods canonical() and declaration() currently can return an invalid cursor and should return an Option instead.

Since this would need a fair number of changes I wanted to confirm with you that this is the right way to go.

@pvdrz
Copy link
Contributor

pvdrz commented Sep 15, 2022

I'd consider this as something important so we don't end up with garbage down the road by accident. However, I wonder if a transition to clang-rs would fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants