Gather comments from headers #141
Conversation
|
/bindings |
|
I urge you to take a cursory look at one of the binding files, and just click the load diff even though it is massive. This is ready for review. I just don't know who cares enough to approve / disapprove of this. |
|
Ooh, I like this. Two points of feedback. One, I note that almost all of the doc comments have a leading space that at least visually looks annoying. If rustdoc is not stripping out this space then it's probably annoying enough to fix. The other thing is just that I'd like to see an extendr build run using these bindings just to run it through the tests and linting etc. Otherwise it looks good and I think it would be valuable for sure. |
|
There is an option to use I have investigated this to see if we had something for #139, but this parse-callback just provides you with this information as it goes through, the name of the macro, and the tokens of the macro, and you ought to do something else with it. There is no return nothing. But that's an aside. |
|
/bindings |
|
For the other point, see extendr/extendr#497, once it finishes whatever it needs to do. |
|
/bindgen |
|
Okay, so the answer is, that this |
|
/bindings |
|
Alright; I'm re-running everything, but it already worked out, and I've addressed comments from you @multimeric. |
|
No, but #141 (comment)
…On Wed, 1 Mar 2023, 00.50 Hiroaki Yutani, ***@***.***> wrote:
***@***.**** approved this pull request.
Looks good to me!
------------------------------
In build.rs
<#141 (comment)>:
> @@ -508,18 +509,24 @@ fn generate_bindings(r_paths: &InstallationPaths, version_info: &RVersionInfo) {
// Finish the builder and generate the bindings.
let bindings = bindgen_builder
+ .generate_comments(true)
+ .parse_callbacks(Box::new(RCallbacks))
+ .clang_arg("-fparse-all-comments")
+ // .clang_arg("-fretain-comments-from-system-headers")
Do we need this line?
⬇️ Suggested change
- // .clang_arg("-fretain-comments-from-system-headers")
—
Reply to this email directly, view it on GitHub
<#141 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIDVSGIHPGL6224JMPRCELWZ2FM7ANCNFSM6AAAAAAVJ47XZM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
So you removed it because it was causing the build to fail on Mac? What value did it provide? I'm sure it's fine to leave for later. |
|
It didn't do anything on the windows part.
But it clearly affects the other platforms. So there could be something
there...
I'll remove it (or either of you can remove the line) and then let's merge
this.
It is not clean to have commented lines or code anyways.
…On Wed, 1 Mar 2023, 04.59 Michael Milton, ***@***.***> wrote:
So you removed it because it was causing the build to fail on Mac? What
value did it provide? I'm sure it's fine to leave for later.
—
Reply to this email directly, view it on GitHub
<#141 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIDVSHEENYT4BWXLJMGPZLWZ3CTNANCNFSM6AAAAAAVJ47XZM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Co-authored-by: Hiroaki Yutani <[email protected]>
|
Urgh I just remembered the changelog. Please add a PR updating it too. |
|
Thanks! Putting it behind |
Adding these lines to the
clang-call enables getting some comments.This is not much, but it is something. Maybe it will be more useful later