Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic pseudo-instantiation macro for container wrapper types #102

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

HertzDevil
Copy link
Contributor

Currently, all instantiated containers must be referred to using their mangled names. This PR provides two ways to name those wrapper types:

  • Container(T): A module which contains no code. It is included by and only by the actual wrapper type with the same type arguments. It can be used as type restrictions.
  • Container.of(T): A macro expression that produces the concrete, non-generic wrapper type. It can be used to construct containers in that wrapper type.

In Qt5.cr only QList and QVector have been wrapped so far. The .of macro for the latter looks like this:

module QVector(T)
  macro of(*type_args)
    {% types = type_args.map(&.resolve) %}
    {% if types == {UInt32} %} {{ Container_QVector_QRgb }}     # macro expansion here turns
    {% elsif types == {Point} %} {{ Container_QVector_QPoint }} # Path into fully qualified TypeNode
    {% elsif types == {PointF} %} {{ Container_QVector_QPointF }}
    {% elsif types == {Float64} %} {{ Container_QVector_qreal }}
    {% elsif types == {UInt32} %} {{ Container_QVector_unsigned_int }}
    {% elsif types == {Float64} %} {{ Container_QVector_double }}
    {% elsif types == {TextLength} %} {{ Container_QVector_QTextLength }}
    {% elsif types == {TextFormat} %} {{ Container_QVector_QTextFormat }}
    {% elsif types == {LineF} %} {{ Container_QVector_QLineF }}
    {% elsif types == {Line} %} {{ Container_QVector_QLine }}
    {% elsif types == {RectF} %} {{ Container_QVector_QRectF }}
    {% elsif types == {Rect} %} {{ Container_QVector_QRect }}
    {% else %} {% raise "QVector(#{types.splat}) has not been instantiated" %}
    {% end %}
  end
end

For the special case of sequential containers, these wrappers also gain a .from constructor, which does exactly the same thing as BindgenHelper.wrap_container, except again no mangled names are used (see below for an example). The .of mechanism is independent of sequential containers.

There are at least two issues that need to be addressed in future PRs:

  • If QRgb is an alias of unsigned int, then QVector<QRgb> is also an alias of QVector<unsigned int>. So far the container processors do not consider these aliases, so they end up instantiating both vector types. (qreal should be turned into an alias as well.) Thus, alias detection should be extended to template arguments.
  • Nested containers like QVector<QVector<int>> can only be obtained with QVector.of(Enumerable(Int32)), not with QVector.of(QVector.of(Int32)), due to the special passing rules of sequential containers. This expression becomes ambiguous if QVector<QList<int>> is also instantiated. Now that container types can be named by the user and that the .from constructor makes Enumerable conversion explicit, I suggest removing those passing rules after this PR, so Crystal arrays must be converted at the call site: (this removal would be a breaking change, so it needs to be accompanied by a version bump)
    class Pen
      # before
      def dash_pattern=(pattern : Enumerable(Float64)) : Void
      # after (using the mangled wrapper type directly is also okay)
      def dash_pattern=(pattern : QVector(Float64)) : Void
    end
    
    def f(pen : Qt::Pen)
      # before
      pen.dash_pattern = [3.0, 1.5, 1.0, 0.5]
      # after
      pen.dash_pattern = Qt::QVector.of(Float64).from [3.0, 1.5, 1.0, 0.5]
    end

@HertzDevil HertzDevil changed the title Pseudo-instantiation macro for generic wrapper types Generic pseudo-instantiation macro for container wrapper types Oct 27, 2020
@Papierkorb
Copy link
Owner

In general, I really like these macros. They're much nicer to use than those mumbo-jumbo generated ones! However:

def f(pen : Qt::Pen)
  # before
  pen.dash_pattern = [3.0, 1.5, 1.0, 0.5]
  # after
  pen.dash_pattern = Qt::QVector.of(Float64).from [3.0, 1.5, 1.0, 0.5]
end

I think the 'after' usage is too complicated. Bindgen strives to produce code that's nice to use to the lib-user (At the expense of ease-of-use as configuration goes at times).

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Oct 28, 2020

Understandable. That said, the breaking change only happens when passing containers to the wrapper; the wrapper should be free to return containers directly, i.e.

class Pen
  # before
  def dash_pattern : Enumerable(Float64)
  # after
  def dash_pattern : QVector(Float64)
end

This might be sufficient to fix the second issue. For it not to break existing code QVector(Float64) < Enumerable(Float64) must be true, but this isn't the case right now because Indexable(Float64) comes from BindgenHelper::SequentialContainer(Float64) instead, and namespaces (modules) cannot include other modules at the moment. So I'll leave this part as is and the patch is ready.

@HertzDevil
Copy link
Contributor Author

QVector.of will look like this after this patch is rebased onto #108:

module QVector(T)
  macro of(*type_args)
    {% types = type_args.map(&.resolve) %}
    {% if types == {UInt32} %} {{ Container_QVector_unsigned_int_ }}
    {% elsif types == {Point} %} {{ Container_QVector_QPoint_ }}
    {% elsif types == {PointF} %} {{ Container_QVector_QPointF_ }}
    {% elsif types == {Float64} %} {{ Container_QVector_double_ }}
    {% elsif types == {TextLength} %} {{ Container_QVector_QTextLength_ }}
    {% elsif types == {TextFormat} %} {{ Container_QVector_QTextFormat_ }}
    {% elsif types == {LineF} %} {{ Container_QVector_QLineF_ }}
    {% elsif types == {Line} %} {{ Container_QVector_QLine_ }}
    {% elsif types == {RectF} %} {{ Container_QVector_QRectF_ }}
    {% elsif types == {Rect} %} {{ Container_QVector_QRect_ }}
    {% else %} {% raise "QVector(#{types.splat}) has not been instantiated" %}
    {% end %}
  end
end

@HertzDevil
Copy link
Contributor Author

Rebased on top of master. Also there is now special logic to prefer container module names (TypeConfig#container_type) over #crystal_type, so nested container names can be disambiguated in the .of macro without the pass behaviour of any containers being altered (see last commit for examples).

Please take a look and see if anything still needs to be changed.

@Papierkorb
Copy link
Owner

This might be sufficient to fix the second issue. For it not to break existing code QVector(Float64) < Enumerable(Float64) must be true, but this isn't the case right now because Indexable(Float64) comes from BindgenHelper::SequentialContainer(Float64) instead, and namespaces (modules) cannot include other modules at the moment. So I'll leave this part as is and the patch is ready.

Couldn't this be done if each specialization would include these modules by themselves?

@HertzDevil
Copy link
Contributor Author

More specifically it could break code that depends on that return type for type deduction, e.g. instance variables: https://play.crystal-lang.org/#/r/9z1p

If T.f returns an Enumerable(Int32) then any of the includes will work, but if it returns Vector(Int32) then only the one include inside Vector itself will work, because T#@x's type is deduced to be whatever T.f's return type restriction is. Thus it needs to be the case that Vector(T) < Enumerable(T).

@Papierkorb
Copy link
Owner

Mh, this works fine in the playground: https://play.crystal-lang.org/#/r/9zap

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.

None yet

2 participants