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

Don't generate bindings for deleted member functions #2044

Closed

Conversation

martinboehme
Copy link
Contributor

See #2043 for details.

@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.

Yeah, this looks sensible to me, thanks! I can't merge right away because github now requires me to approve CI for new contributors, but if it's green I'm happy to merge as is. Though I have a minor suggestion to maybe do a bit less work in the common case.

@@ -597,6 +597,10 @@ impl ClangSubItemParser for Function {
return Err(ParseError::Continue);
}

if cursor.is_deleted_function() {
return Err(ParseError::Continue);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth to shuffle the logic a bit to do less work in the common case, something like:

if cursor.is_inlined_function() {
    if !context.options().generate_inline_functions {
        return Err(..);
    }
    if cursor.is_deleted_function() {
        return Err(..);
    }
}

@emilio
Copy link
Contributor

emilio commented Apr 30, 2021

CI looks green except for rustfmt, so will merge with that fixed and my nit addressed. Thanks!

@emilio emilio closed this in 910d2be Apr 30, 2021
@martinboehme
Copy link
Contributor Author

Awesome! Was about to push fixes, but you were faster than me. Thanks for fixing those two nits for me!

LoganBarnett pushed a commit to LoganBarnett/rust-bindgen that referenced this pull request Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants