Papierkorb / bindgen

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

Generate property methods for accessible instance variables #72

Closed HertzDevil closed 4 years ago

HertzDevil commented 4 years ago

Resolves #71. The integration test generates the following extra methods in the Crystal wrappers:

module Test
  class Point
    def x() : Int32 end
    def x=(x : Int32) : Void end
    def y() : Int32 end
    def y=(y : Int32) : Void end
  end

  class Props
    def x_pub() : Int32 end
    def x_pub=(x_pub : Int32) : Void end
    def y_pub() : Int32 end
    def position_ptr() : Point end
    def position_ptr=(position_ptr : Point) : Void end
    def position_val() : Point end
    def position_val=(position_val : Point) : Void end
    private def x_prot() : Int32 end
    private def x_prot=(x_prot : Int32) : Void end
    private def y_prot() : Int32 end
  end
end

Some implementation notes:

The YAML config will probably be something like this:

types:
  Props:
    instance_variables:
      # renames the property methods; regex expressions are supported
      "^(.)_pub$": { name: "\\1_public" }

      # this will hide all the property methods
      ".*": { ignore: true }

      # do not allocate new instances when accessing and modifying through a pointer member
      # (e.g. intrusive linked lists; I haven't figured out the proper semantics for this)
      # "position_ptr": { weak: true }

      # allow null pointers
      "position_ptr": { nilable: true }
Papierkorb commented 4 years ago

Hello,

Good stuff again 👍 . I know the YAML snippet is a draft, but it looks good to me - However, I'd change name to rename to make it more explicit.

Maybe it'd be possible to allow instance_variables: false | true as well, with true being the default using the default operations for everything. false would disable the feature for the type and, if the user wants to whack the structure a bit, use the hash-structure to configure explicit field types like in your snippet.

In general, you're working fast so I figure doing in-depth code-review before you're satisfied (Hence the "draft") would waste your and my time. Please just say in your comments that it's ready and I'll have a look :)

Cheers!

HertzDevil commented 4 years ago

Both this patch and the superclass one are ready. (There will be a few merge conflicts, though I made sure they can be resolved.) I also finally tested this on qt5.cr, and rather unsurprisingly it generates exactly zero getters and setters unless I add QStyleOption and its subclasses.

Bindgen::Crystal::Pass#wrapper_initialize_template was modified as part of the changes to support nilable pointer members, and this incidentally affected a few virtual overrides. For example:

module Qt
  abstract class GraphicsItem
    def initialize(parent : GraphicsItem? = nil)
      jump_table = Binding::BgJumptable_GraphicsItem.new(
        bg_QGraphicsItem_paint_QPainter_X_const_QStyleOptionGraphicsItem_X_QWidget_X:
          BindgenHelper.wrap_proc({% if forwarded.includes?("paint") %}
            Proc(Binding::QPainter*, Binding::QStyleOptionGraphicsItem*, Binding::QWidget*, Void).new{|painter, option, widget|
              self.paint(Painter.new(unwrap: painter), option, widget.try {|ptr| Widget.new(unwrap: ptr) unless ptr.null?})
              # before 07be350: self.paint(Painter.new(unwrap: painter), option, Widget.new(unwrap: widget))
            }
          {% else %}
            nil
          {% end %}),
        # ...
      )
    end

    abstract def paint(painter : Painter, option : Binding::QStyleOptionGraphicsItem*, widget : Widget? = nil) : Void
  end
end

I believe the new behaviour is in fact the correct one, since it looks like the old code would create a Widget with a null pointer.

Papierkorb commented 4 years ago

Rebased this one locally, closing and will push your changes in a sec.