Skip to content

ClangImporter: use nameless arguments for anonymous struct/unions in constructors. #6985

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

Conversation

Fruneau
Copy link
Contributor

@Fruneau Fruneau commented Jan 23, 2017

This patch follows that discussion: #6816 (review) and is a repush of #6935 against master.

Following a13c134, constructors of structures/unions containing
anonymous structures/unions fields include those field in their parameter
list, using the generated field name as parameter name:

 typedef struct foo_t {
     union {
         int a;
         int b;
     };
 } foo_t;

Generates:

 struct foo_t {
     init(__Anonymous_field0: foo_t.__Unnamed_union__Anonymous_field0)
 }

 let foo = foo_t(__Anonymous_field0: .init(a: 1))

One important downside here is that the generated field name get exposed
in the API.

An idealistic approach would be to generate the constructors that expose
the fields indirectly inherited from those structures:

 struct foo_t {
     init(a: Int32)
     init(b: Int32)
 }

However, this approach requires the generation of a constructor per valid
combination of indirect fields, which might start having a huge
cardinality when we have nested anonymous structures in nested anonymous
unions...

 typedef struct bar_t {
     union {
         struct {
             int a;
             int b;
         };
         struct {
             int c;
             int d;
         };
     };
     union {
         int e;
         int f;
     };
 } bar_t;

In this examples, we have 4 constructors to generates, for (a, b, e), (a, b, f),
(c, d, e) and (c, d, f).

The proposed approach is to use a nameless parameter for anonymous
structures/unions, still forcing the user to build that sub-object by
hand, but without exposing the generated field name. This is very similar
to what can be done in C:

 foo_t foo = { { .a = 1 } };
 let foo = foo_t(.init(a: 1))

Or

 bar_t bar = { { { .a = 1, .b = 2 } }, { .e = 1 } };
 let bar = bar_t(.init(.init(a: 1, b: 2)), .init(e: 1))

…constructors.

Following a13c134, constructors of structures/unions containing
anonymous structures/unions fields include those field in their parameter
list, using the generated field name as parameter name:

 typedef struct foo_t {
     union {
         int a;
         int b;
     };
 } foo_t;

Generates:

 struct foo_t {
     init(__Anonymous_field0: foo_t.__Unnamed_union__Anonymous_field0)
 }

 let foo = foo_t(__Anonymous_field0: .init(a: 1))

One important downside here is that the generated field name get exposed
in the API.

An idealistic approach would be to generate the constructors that expose
the fields indirectly inherited from those structures:

 struct foo_t {
     init(a: Int32)
     init(b: Int32)
 }

However, this approach requires the generation of a constructor per valid
combination of indirect fields, which might start having a huge
cardinality when we have nested anonymous structures in nested anonymous
unions...

 typedef struct bar_t {
     union {
         struct {
             int a;
             int b;
         };
         struct {
             int c;
             int d;
         };
     };
     union {
         int e;
         int f;
     };
 } bar_t;

In this examples, we have 4 constructors to generates, for (a, b, e), (a,
b, f), (c, d, e) and (c, d, f).

The proposed approach is to use a nameless parameter for anonymous
structures/unions, still forcing the user to build that sub-object by
hand, but without exposing the generated field name. This is very similar
to what can be done in C:

 foo_t foo = { { .a = 1 } };
 let foo = foo_t(.init(a: 1))

Or

 bar_t bar = { { { .a = 1, .b = 2 } }, { .e = 1 } };
 let bar = bar_t(.init(.init(a: 1, b: 2)), .init(e: 1))

Signed-off-by: Florent Bruneau <[email protected]>
@jrose-apple
Copy link
Contributor

@swift-ci Please test

@slavapestov slavapestov merged commit 56189ad into swiftlang:master Jan 24, 2017
@slavapestov
Copy link
Contributor

@Fruneau Does this need a CHANGELOG.md update as well?

Also do you mind cherry-picking this to swift-3.1-branch and creating a PR for that also? We don't want swift-3.1-branch to have a different implementation of anonymous structs/unions than master, since this feature was just introduced.

@Fruneau
Copy link
Contributor Author

Fruneau commented Jan 24, 2017

@slavapestov AFAICT, #6935 already covers the need of a cherry-pick on swift-3.1-branch.

For the changelog, I could add a note about the constructor in the section about the indirect fields, but I'm not sure it's actually needed. What's your opinion on this?

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