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

Add @[Extern] attribute #3051

Closed
wants to merge 1 commit into from
Closed

Add @[Extern] attribute #3051

wants to merge 1 commit into from

Conversation

asterite
Copy link
Member

Fixes #2422

This PR works but I want to discuss this a bit before merging this.

Right now lib structs, unions and funs are restricted to either primitive types (Int32, Float32, etc.), pointers, enums, and structs and unions inside lib types. You can't use a "regular" crystal class or struct.

With this PR one can now mark a crystal struct (defined outside a lib declaration) as @[Extern], and then the compiler lets you use this type in the above places.

lib structs and unions automatically receive getters and setters for each field, and they get an empty constructor that sets the type to zero (memzero):

lib LibFoo
  struct Foo
    x : Int32
    y : Int32
  end
end

foo = LibFoo::Foo.new
p foo # => LibFoo::Foo(@x=0, @y=0)

In contrast, and this is what I'd like to discuss, a regular struct marked with @[Extern] follows crystal rules, and the compiler will complain if declaring an instance variable and not properly initializing it. However, one can use @x = uninitialized ... to mark that variable as "unsafe" and not needing a check from the compiler.

Alternatively, we could automatically generate getters and setters for every instance variable, but I feel this is less safe and nice. One disadvantage of not doing this is that for C struct/union getters the compiler will invoke to_unsafe to do numeric conversions, but without these getters one would have to do these conversions.

But, I think that this extern structs will probably be used with types that are created by lib functions, and you rarely need to set their instance variables, but instead you query them (or maybe you use them but don't let users query them). And in any case you can define setters and do a manual conversion there, if needed.

One can also mark a struct with @[Extern(union: true)], and now the struct will behave as a C union. For example:

@[Extern(union: true)]
struct Int64OrFloat64
  @int = uninitialized Int64
  @float = uninitialized Float64

  property int
  property float
end

x = Int64OrFloat64.new
x.int = 10_i64
p x # => Int64OrFloat64(@int=10, @float=4.9406564584124654e-323)

With this new @[Extern] attribute writing C bindings that have lots of structs and unions is greatly simplified, as one doesn't need to wrap these in a Crystal struct. There is also no need to define a to_unsafe because the type is already the type that's passed directly to C. I checked if this could be applied to, for example, the LLVM bindings and yes, and many LLVM:Value.new(LibLLVM.get_some_value) are replaced with just LibLLVM.get_some_value. But in this case the gain is very little. I think libraries such as SDL or SFML will benefit more from this because they expose structs that can be accessed directly, not only via an opaque pointer.

One missing thing mentioned in #2422 is that, in order to keep binary compatibility with the C library, one shouldn't be able to add more instance variables to such structs. We could trust the user not to do so, or simply allow instance variable declarations on the first definition of the type, disallowing adding more instance variables later. For now we can start with the first option and eventually implement the second for a more robust solution.

Thoughts?

@oprypin
Copy link
Member

oprypin commented Jul 31, 2016

This sounds very nice and flexible to me. But a problem would be chained assignment, which works with lib-structs if I remember correctly but is impossible to implement with normal structs.

Verified

This commit was signed with the committer’s verified signature.
headius Charles Oliver Nutter
@oprypin
Copy link
Member

oprypin commented Aug 18, 2016

Tried this out in a project, works well.

@asterite
Copy link
Member Author

@BlaXpirit Nice! Care to show some code?

@oprypin
Copy link
Member

oprypin commented Aug 18, 2016

@asterite https://github.com/BlaXpirit/crystal-chipmunk/blob/master/src/chipmunk/structs.cr


Hm, I realized that this code probably only uses the simplest Vect type, so then I tried out p ball_shape.point_query(CP.v(1, 1)) in the example, and that still works and looks correct.

@oprypin
Copy link
Member

oprypin commented Sep 2, 2016

I've continued work on my library (using this branch), and everything runs without a slightest problem, even in complex configurations.

I hope this will be merged.

@ysbaddaden
Copy link
Contributor

I think it's a good addition to the language. We could have Crystal structs mapping C structs, with nice methods for example. Or cast Slices to a Crystal structs (kinda unsafe but fast). It's down level, but in a good way: more control of the memory layout, and nicer interaction with external libraries.

@oprypin
Copy link
Member

oprypin commented Sep 4, 2016

There seems to be a problem with named arguments to initialize.

@[Extern]
struct A
  def initialize(x : Int32)
    @p = x
  end
end

A.new(x: 6)
Error in ./test.cr:8: wrong number of arguments for 'A.new' (given 0, expected 1)
Overloads are:
 - A.new(x : Int32)

A.new(x: 6)
^

@oprypin
Copy link
Member

oprypin commented Sep 9, 2016

@asterite, what's the status on this? Will this ever be merged?
I would really like this feature, and my project can't go on without it. And it's such a pain especially now that the latest Crystal release can't compile this branch.

@asterite
Copy link
Member Author

asterite commented Sep 9, 2016

@BlaXpirit this needs more thought. Right now all C types live under lib, this makes it possible to use some lib types outside lib. Maybe instead of an attribute it could be done in another way, but I have to sit down and review this with others and see if this is the best approach to this.

@oprypin
Copy link
Member

oprypin commented Sep 9, 2016

These are not "lib types outside lib", these are just structs that can't be extended.

I agree that the currently taken approach to lib makes this change look out of place, but there is nothing wrong with the change.

I wrote a comment in the issue.

@asterite asterite added this to the 0.19.2 milestone Sep 14, 2016
@asterite asterite closed this in dcfd903 Sep 14, 2016
@asterite asterite deleted the feature/extern2 branch September 15, 2016 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants