-
Notifications
You must be signed in to change notification settings - Fork 770
codegen: Deduplicate derive traits added by add_derives() parse callback #3296
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
base: main
Are you sure you want to change the base?
Conversation
680406f
to
52081ca
Compare
5a2c0f4
to
aa5e696
Compare
Is bindgen effectively unmaintained? What can I do to get this PR merged? Is this a controversial change? If so, can I have some feedback? |
Bindgen is a free-time project for me, this PR was opened two weeks ago. Please be patient because I unfortunately don't have a lot of free time lately :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but I don't think the hash set is going to be particularly worth it for derive lists in particular, and not doing that would simplify the code a bit, wdyt?
LMK if you disagree tho.
where | ||
I: Iterator<Item = &'a str>, | ||
{ | ||
// Use a HashSet to track already seen elements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
derive lists are usually pretty small, I don't think the copy + hashset overhead is going to be particularly worth it in this case...
That way the is_empty check on the caller can go. WDYT?
.derive_partialord(false) | ||
.derive_ord(false) | ||
.derive_hash(false) | ||
.parse_callbacks(Box::new(AddDerivesCallback::new(add_derives))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be less code to extend bindgen-integration perhaps? But this is fine.
Fixes #3286