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

Preliminary support for C arrays #76

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

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Aug 18, 2020

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

  • 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.
  • 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 *.

@Papierkorb
Copy link
Owner

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!

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Aug 21, 2020

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
Copy link
Contributor Author

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 #<<).

@Papierkorb
Copy link
Owner

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.

@HertzDevil
Copy link
Contributor Author

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).

@Papierkorb
Copy link
Owner

Could you resolve the conflict? LGTM after that 👍

@docelic
Copy link
Collaborator

docelic commented Jul 17, 2021

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

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.

3 participants