Add a callback for function-like macros

[kulp]

Closes #1792.

Because whitespace information is not represented explicitly during parsing, it is non-trivial to reconstruct the exact textual form of the macro from the token list. I had previously inserted whitespace between tokens, which I still believe would be correct and acceptable, but it seems that spelling the tokens separately would be better.

Given this input :

#define jit_get_note(n,u,v,w)	_jit_get_note(_jit,n,u,v,w)

and this callback :

fn func_macro(&self, name: &str, value: &[&[u8]]) {
    dbg!(name);
    dbg!(value);
}

this sort of output is achieved (pseudo-code) :

name = "jit_get_note(n,u,v,w)"
value = &[ "_jit_get_note", "(", "_jit", ",", "n", ",", "u", ",", "v", ",", "w", ")" ]

[emilio]

Thanks! This looks basically fine. I have some questions, and I think that we should avoid re-tokenizing every macro twice.

[emilio]

Also, why does handle_function_macro take a closure instead of just a &dyn ParseCallbacks or such?

[kulp]

Also, why does handle_function_macro take a closure instead of just a &dyn ParseCallbacks or such?

I had previously passed in &dyn ParseCallbacks as you say, and I will happily change it back if you like. I simply thought that avoiding exposing ParseCallbacks to handle_function_macro lowered the risk of undesirable coupling and made handle_function_macro more composable, but that is probably a philosophy taken a little too far.

[kulp]

@emilio, I believe I have addressed all your questions and concerns. I no longer re-tokenize twice, and I think I have demonstrated that from_utf8 is not called more times than before the current PR.

Please let me know what else I can do.

[kulp]

@emilio, I was not sure if the right etiquette was to mark the conversations as resolved, or to leave them for you to do so -- I marked them as resolved.

Is there anything else this PR needs ?

[emilio]

No, marking as resolved is all fine. Sorry, just have to get to reviewing this properly again (bindgen is not my $dayjob :)).

Sorry for the lag and thanks for updating the patch.

[kulp]

No, marking as resolved is all fine. Sorry, just have to get to reviewing this properly again (bindgen is not my $dayjob :)).

Sure, I understand. No rush -- I just do not know whether things get missed and how much active pushing you want from me. I will come back in a few weeks.

[bors-servo]

☔ The latest upstream changes (presumably 2ba27bf) made this pull request unmergeable. Please resolve the merge conflicts.

[emilio]

This looks basically ok to me, with a few minor changes.

[kulp]

☔ The latest upstream changes (presumably 2ba27bf) made this pull request unmergeable. Please resolve the merge conflicts.

I have rebased and force-pushed. I also ensured that every commit passes cargo test both in the root directory and in bindgen-integration, so I can back up to an earlier commit if I have gone too far with my refactoring.

[emilio]

Thanks, this looks good to me! Mind adding that minor comment?

I think squashing the commits would also be great, 52 commits may be a bit too much for this. Your call whether you want to leave it all in one commit, or put the implementation and tests in different commits or what not. (Or I can just squash them before merging).

Thanks again!

[kulp]

Thanks, this looks good to me! Mind adding that minor comment?

Certainly, I will do that next. I did not see your quick review response, or I would have held off on my last push.

I think squashing the commits would also be great, 52 commits may be a bit too much for this. Your call whether you want to leave it all in one commit, or put the implementation and tests in different commits or what not. (Or I can just squash them before merging).

I will try to squash down to ten meaningful commits or so.

Thanks again!

Thanks for working with me on this ! It has been a pleasure.

[kulp]

I am as done as I know how to be. I mistakenly re-requested review from you, @emilio.

Thanks for your help.

[emilio]

Sweet, thank you!

[kulp]

@emilio, I could use this feature better in lightning-sys, if a new release were published to crates.io in a week or two. Is that reasonable ?

[emilio]

Yes, I'll try to get a release done soonish. Please nag if it's not done by the end of this week :)

[kulp]

Yes, I'll try to get a release done soonish. Please nag if it's not done by the end of this week :)

@emilio, here is your nagging :) although it is really not a big deal to me if it takes a few weeks.

Let me know if I can help, for example by writing up a Changelog entry.

[emilio]

Just published a new release. Thanks for all the contributions you've sent so far @kulp!