-
Notifications
You must be signed in to change notification settings - Fork 748
Add Builder functions for including folder(s) and headers #2123
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
Conversation
@emilio for your review or referral to a reviewer |
@aatifsyed it has been a while so the logs are gone. Do you happen to know why CI was failing? Regarding the PR itself. I'd use |
1846a9d
to
7867787
Compare
@pvdrz Thanks for resurrecting this!
|
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.
Maybe we can change
includes
by something less similar toinclude
likeinclude_folder
or something? I'm bad at naming things so deferring to @emilio or @kulp for a review.
I am probably not better than @pvdrz at naming things. I agree that include
and includes
are too similar; but the new function headers
is too close to the existing function header
, too.
The only thing that comes to mind is include_multiple
and header_multiple
but I do not really like those either.
I am not standing in the way of this but for the moment I am a little wary of cluttering the API.
src/lib.rs
Outdated
} | ||
|
||
/// Include multiple folders for system header file resolution | ||
pub fn includes<T: AsRef<str>>( |
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.
Personally I think having both includes
and include
is probably not necessary, except for consistency with headers
and header
; if we were starting from scratch, I would prefer just to have the plural versions, and call them with an array of size one, or std::iter::once
, if I have only one element. But maybe that is just me.
But removing header
would break backward compatibility, and having both header
and headers
but only includes
seems inconsistent, so maybe I do not have anything better to suggest here.
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.
include_directories
?
maybe we can use |
☔ The latest upstream changes (presumably #2284) made this pull request unmergeable. Please resolve the merge conflicts. |
Hello all, thanks for your feedback :) I'm away from my PC for a couple of weeks, so I'll catch up and amend when I'm back. I'd be interesting in feedback on the following suggestion:
So the documented API remains smaller? We could even |
I'd favor |
☔ The latest upstream changes (presumably 2034a0f) made this pull request unmergeable. Please resolve the merge conflicts. |
See #2743. |
:/ |
Possibly I was too picky about the naming of things. The bindgen project has varying amounts of volunteer support; sometimes one person gets lucky, and another unlucky. Still, I am sorry, @aatifsyed, that your PR did not get merged. |
I find myself frequently extending
bindgen::Builder
with the following functions:This pull request adds them.
Points to consider
-I
vs--include-directory=
pkg-config
, a motivating usecase forincludes