bindgen icon indicating copy to clipboard operation
bindgen copied to clipboard

Preliminary support for C arrays

Open HertzDevil opened this issue 3 years ago • 7 comments

Adds support for C array types, i.e. int [4][3][2]. It is not to be confused with std::array.

  • [x] Allow StaticArray members in lib structs generated by copy_structure: true. Multi-dimensional C arrays are also supported. VLAs are rejected by SanityCheck, because Crystal doesn't support them either.
  • [x] Generate instance getter methods for array members, which should always return a Slice (read-only if the array fields are const). There will be no corresponding setters.

Optional features:

  • Allow wrapper methods to take array arguments or return arrays. They are wrapped in Slice if the bounds are known, and Pointer otherwise.
  • The Clang parser's output format cannot distinguish between an array of pointers (int *x[4]) and a pointer to an array (int (*x)[4]). This patch assumes the former as it is way more common in C code than the latter, although in C++ int (&x)[4] would translate to the latter. The two can be differentiated by treating every array / pointer layer as a pseudo-generic type, similar to Bindgen::Parser::Type#template. The downside is that each layer creates a new type.
  • The parser might need to distinguish between const pointers and pointers-to-const as well.
  • Map C array constants to Crystal Arrays, provided they only contain other supported constants (Int | Float | String | Bool).

This patch also fixes what I believe to be a bug where CopyStructs converts void ** fields to a plain void *.

HertzDevil avatar Aug 18 '20 18:08 HertzDevil

Map C array constants to Crystal Arrays, provided they only contain other supported constants (Int | Float | String | Bool).

I don't think we need that, Slices are fine. They're easy to get into an array if needed, otherwise bindgen should prefer lower overhead. Could be an opt-in feature however.

Good stuff as always, cheers!

Papierkorb avatar Aug 18 '20 20:08 Papierkorb

About struct CrystalSlice: When I tried to attach the element type to it as a template parameter, I got errors like this:

instance_properties.cpp:57:30: error: 'bg_Props_v_GETTER_' has C-linkage specified, but returns incomplete type 'CrystalSlice<int>' which could be incompatible with C [-Werror,-Wreturn-type-c-linkage]
extern "C" CrystalSlice<int> bg_Props_v_GETTER_(Props * _self_) {

How did CrystalProc get around this?

Also it seems the PR is already quite large, so I'm calling it done once I add the documentation. General methods could wait a bit longer. (Most C array arguments in practice are just T [] => T * anyway.)

HertzDevil avatar Aug 21 '20 13:08 HertzDevil

Putting this PR on hiatus because there is one major problem: slices can only refer to binding types even when the element type is wrapped. For example:

struct Inner {
  char a;
};

struct Outer {
  Inner b[4];
  Inner *c[5];
};
module Test
  lib Binding
    alias Inner = Void
  end

  class Inner
    @unwrap : Binding::Inner*

    def initialize(unwrap : Binding::Inner*)
    end
  end

  class Outer
    def b() : Slice(Binding::Inner)
      # Slice(Void) not allowed
      # copying Inner's structure might work
    end

    def c() : Slice(Binding::Inner*)
      # ::Slice#[] returns a raw pointer; it won't call Inner#initialize
    end
  end
end

One possible solution is to return a custom Slice-like type instead of the core library's one. This in turn could leverage BindgenHelper::SequentialContainer, but at this moment I don't have any plans of getting that to work (at the very least the custom Slice would not have #<<).

HertzDevil avatar Aug 28 '20 16:08 HertzDevil

That sounds reasonable, still dang, didn't think about that either. I think that indeed, a custom Enumerable will be what we want for this.

Papierkorb avatar Aug 28 '20 17:08 Papierkorb

That's it for now, Bindgen will currently reject arrays that are not built-in types (more specifically getter methods are omitted, and lib structs containing them are reported by SanityCheck).

HertzDevil avatar Sep 01 '20 06:09 HertzDevil

Could you resolve the conflict? LGTM after that 👍

Papierkorb avatar Sep 01 '20 16:09 Papierkorb

I'll try to resolve the conflicts here during this weekend; @HertzDevil if you have any comments/notes re. that, please share. Thanks!

docelic avatar Jul 17 '21 08:07 docelic