Skip to content

Conversation

@HertzDevil
Copy link
Contributor

This patch, split from #78, promotes conversion templates (Bindgen::Call::Result#conversion) to their own class hierarchy. This allows templates to perform complex substitutions that Util.template cannot achieve.

The main issue with Util.template is that it only supports simple string substitutions (e.g. %.to_unsafe, Converter.unwrap(%)). Passing a wrapper-Proc to a CrystalProc is an example that cannot be expressed in this way:

  • The binding expects a binding-Proc, which takes binding types and returns a binding type;
  • The user-provided wrapper-Proc takes and returns wrapper types.
  • Thus, the binding-Proc needs to convert each argument to its wrapper type, invoke _proc_, then convert the return value to its binding type;
  • The binding-Proc itself needs to be converted into its binding type (CrystalProc).

Since the conversion must occur every time a wrapper-Proc is passed to the binding, the best place to perform the conversion is through the template facility, rather than in a Call::Body. However, as noted in the original bug report, such Proc conversions are already properly handled in the jumptables (Processor::VirtualOverride::JumptableHook), just not in the Qt processor. Therefore, the template for wrapper-Procs actually ends up using CallBuilder::CrystalFromCpp directly; it works because that Call::Body generates an expression, rather than a complete code block.

Apart from Proc, I expect that some other generic types will also require extra conversions beyond Util.template's capability. This is why I suggested that Slice may also use this feature as well.

Template::None and Template::Sequence are utility templates that either perform no substitutions at all, or chain multiple templates together. They make the composition of templates more concise. (Now that I think about it, Bindgen::Call::Result#conversion itself could be an Array(Template::Base) too, although #followed_by will be replaced with array concatenation.)

Copy link
Owner

@Papierkorb Papierkorb left a comment

Choose a reason for hiding this comment

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

Please have a look at the comments. Overall this is much much more manageable for me, thanks for splitting it up!

@HertzDevil
Copy link
Contributor Author

Please take a look if anything is still missing.

I rebased the original CrystalProc fix onto my branch, ready for a new PR, by the way: https://github.com/HertzDevil/bindgen/tree/fix-crystalproc-template

@Papierkorb Papierkorb merged commit d76516d into Papierkorb:master Aug 28, 2020
@Papierkorb
Copy link
Owner

Oops, thanks for the ping

@HertzDevil HertzDevil deleted the conversion-templates branch August 29, 2020 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants