Papierkorb / bindgen

Binding and wrapper generator for C/C++ libraries
GNU General Public License v3.0
179 stars 18 forks source link

Add type tags for block parameter disambiguation #60

Closed HertzDevil closed 4 years ago

HertzDevil commented 4 years ago

An attempt to fix #59, through a new BlockOverloads processor. For instance this is Qt::ButtonGroup#on_button_toggled, which has 2 overloads each taking two parameters in the block:

module Qt
  # lib Binding is unmodified

  class ButtonGroup
    def on_button_toggled(_type1_ : AbstractButton.class, _type2_ : Bool.class, &_proc_ : Proc(AbstractButton, Bool, Void)) : SignalConnection
      SignalConnection.new(unwrap: Binding.bg_QButtonGroup_CONNECT_buttonToggled_CrystalProc_void_QAbstractButton_X_bool(self, BindgenHelper.wrap_proc(_proc_)))
    end

    def on_button_toggled(_type1_ : Int32.class, _type2_ : Bool.class, &_proc_ : Proc(Int32, Bool, Void)) : SignalConnection
      SignalConnection.new(unwrap: Binding.bg_QButtonGroup_CONNECT_buttonToggled_CrystalProc_void_int_bool(self, BindgenHelper.wrap_proc(_proc_)))
    end
  end
end

btn_group = Qt::ButtonGroup.new

btn_group.on_button_toggled(Int32, Bool) { |id, checked| typeof(id) }
btn_group.on_button_toggled(Int32, Bool, &->(id, checked) { typeof(id) })
# Int32

btn_group.on_button_toggled(Qt::AbstractButton, Bool) { |btn, checked| typeof(btn) }
# Qt::AbstractButton

btn_group.on_button_toggled { |id, checked| }
# Error: wrong number of arguments for 'Qt::ButtonGroup#on_button_toggled' (given 0, expected 2)

Similar transformations may be applied even for non-Qt bindings, as long as block arguments are emitted somewhere (that's why the code sits in a new processor rather than in Qt).

I have restricted this to methods that do not take non-block arguments, because I am unsure whether those type tags should be prepended at the start or inserted right before the block; the former resembles C++ function templates, while the latter places the tags closer to the block itself. Fortunately, all those ambiguous methods in the Qt bindings are precisely those #on_SIGNAL functions, which indeed take just the block argument.

Bindgen::Call::TypeArgument is there because I don't think Argument really supports this _.class usage. This may prove useful for function template instantiations in the future where type tags are similarly passed as regular arguments (template <class> bool QVariant::canConvert() const for example).

docelic commented 4 years ago

@HertzDevil this is awesome, thanks! @Papierkorb the patch looks pretty good to me. When you get a chance please review it from your wider perspective, thanks!

Papierkorb commented 4 years ago

Thank you for writing the code! Just got home from a vacation, please give me a day or two to review it. The summary on your post sounds good to me 👍

HertzDevil commented 4 years ago

The new integration tests are still restricted to Qt signals, because no other processors emit Crystal wrappers expecting block arguments.

Papierkorb commented 4 years ago

I added a comment to your most recent commit. A C++ method that takes a std::function<> as last argument is supposed to turn into a block-receiving method in Crystal - But I'm fine with the placement of the test.

Besides the comment this LGTM, so afterwards I think it's ready to be merged :smile:

docelic commented 4 years ago

Wonderful, thanks!

Papierkorb commented 4 years ago

Ya great job, thank you!