Skip to content

Add opaque syntax that allows declarations #6421

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

Merged
merged 7 commits into from
Oct 7, 2020

Conversation

tadeokondrak
Copy link
Contributor

@tadeokondrak tadeokondrak commented Sep 25, 2020

Implements #5574 (also #4638, I had searched for "declarations" and didn't find anything).
These are not accepted proposals, so this PR might be rejected for that reason. I don't really mind.
I think it's hard to judge whether this feature is a good idea or not without experimenting with an implementation first.

Includes a zig fmt auto-fix because @Type(.Opaque) is no longer valid.

@brodeuralexis
Copy link
Contributor

brodeuralexis commented Sep 25, 2020

This looks nice, but I have a few questions.

  1. Would opaque struct be more consistent as we already have packed struct and extern struct (I also like it more as it makes it more explicit that it is a struct is being declared) ?
    • Is opaque union something we would want ?
  2. Are struct properties private for parent structs (I assume, but I don't see any tests for that) ?
    • Allow pub before struct properties to expose them to parent structs ?
    • Would it make more sense to make opaque the default behaviour for struct and allow pub before properties, exposing them to parent structs, to ensure there is only one way to create structs ?

I use parent struct to mean the struct in which another struct, the child struct is declared, but please correct me if the nomenclature is incorrect.

@tadeokondrak
Copy link
Contributor Author

Would opaque struct be more consistent as we already have packed struct and extern struct (I also like it more as it makes it more explicit that it is a struct is being declared) ?

I don't think it has anything in common with structs.

Is opaque union something we would want ?

C allows both opaque unions and opaque structs, but it doesn't actually matter to code which one is used, since you can't access anything from an opaque type.

Are struct properties private for parent structs (I assume, but I don't see any tests for that) ?

Allow pub before struct properties to expose them to parent structs ?

Would it make more sense to make opaque the default behaviour for struct and allow pub before properties, exposing them to parent structs, to ensure there is only one way to create structs ?

Opaque types can't have fields, and any declarations have the same rules as other containers

@ifreund
Copy link
Member

ifreund commented Sep 26, 2020

As mentioned in #4638 (comment), I recently started work on idiomatic zig bindings for libwayland. These bindings are still in a highly experimental state and are not complete, but nonetheless they provide a good opportunity to test out this new feature as libwayland makes heavy use of opaque pointers.

My initial impressions using this feature are entirely positive: removing the need for wrapper structs to add "methods" to opaque types cleaned up and simplified things significantly. You can see the initial commit migrating to this feature here: ifreund/zig-wayland@bf06840. I intend to move forward with the development of the bindings on that branch for the time being in order to continue to test the new syntax and because the new syntax is much nicer for this use case.

@Rocknest
Copy link
Contributor

Certainly a nice addition however it looks a bit odd to me, especially *opaque {}. Maybe it actually should be a special-cased struct.

Or construct it from comptime list of function pointers (do we need constants in opaque types?), and use it something like this:

fn consume(self: anytype, data: []const u8) void {}
const Consumer = std.meta.Opaque(.{&consume});

@ifreund
Copy link
Member

ifreund commented Sep 26, 2020

Certainly a nice addition however it looks a bit odd to me, especially *opaque {}. Maybe it actually should be a special-cased struct.

It’s nothing like a struct though as it cannot have fields and may hide literally any type including structs, unions, etc.

Or construct it from comptime list of function pointers (do we need constants in opaque types?), and use it something like this:

That looks much more awkward to use. And yes, in my opinion opaque types should allow all kinds of Decls, not just functions. As seen in the diff I posted above I already rely on non-function Decls in my opaque types.

@Rocknest
Copy link
Contributor

@tadeokondrak consider making curly braces optional when the body of an opaque type is empty. (const T = *opaque;)

@tadeokondrak
Copy link
Contributor Author

@Rocknest Hmm, I like how that looks, but it feels confusing that a single keyword creates a new type every time it's used.
opaque == opaque would be false.

@ifreund
Copy link
Member

ifreund commented Sep 26, 2020

@Rocknest Hmm, I like how that looks, but it feels confusing that a single keyword creates a new type every time it's used.
opaque == opaque would be false.

This is already the case for opaque {} == opaque {} no?

Nevertheless, I'd be against making the braces optional. They aren't much pain to type and leaving them out wouldn't be worth the inconsistency/complexity IMO.

@tadeokondrak
Copy link
Contributor Author

This is already the case for opaque {} == opaque {} no?

Yeah, but that holds for struct {}, enum {}, and union {} too, but not for any other non-braced type.

@Rocknest
Copy link
Contributor

Rocknest commented Sep 26, 2020

There is nan that is not equal to self, which is far weirder behaviour if you ask me

@ifreund
Copy link
Member

ifreund commented Sep 26, 2020

By the way, the test failure looks legit: https://dev.azure.com/ziglang/zig/_build/results?buildId=9322&view=logs&j=9512b82f-d185-50ea-ee23-d010bc14782f&t=ca25486c-bdcc-50c6-de82-a9ccd6b64318&l=73738

@tadeokondrak
Copy link
Contributor Author

As another sample, here's zig_clang/translate_c converted to use it: tadeokondrak@8dd2a03

@tadeokondrak tadeokondrak force-pushed the opaque-syntax branch 2 times, most recently from 6eacae5 to c266a70 Compare October 2, 2020 15:10
@katesuyu
Copy link
Contributor

katesuyu commented Oct 7, 2020

This would be absolutely amazing if merged. Not only for C bindings, but for any case where you need to represent a handle to a data structure that does not operate in terms of fields (in the traditional sense, at least), but still needs ABI-compatible pointers: all without losing the benefits method call syntax allows!

@andrewrk andrewrk merged commit 95a3737 into ziglang:master Oct 7, 2020
@zigster64
Copy link
Contributor

zigster64 commented Oct 7, 2020

+1 from me.
Im only a week in on my Zig adventure, but Ive already run into this one hard, and the suggested approach above is a godsend.

Looking through the linked wayland wrapper using the opaque fork, its really simple to reason about, and much less cluttered. I like.

lol - merged, beat me to it :)

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.

7 participants