enum: Support inlined definitions for tuple variants with a single field #631

petrochenkov · 2020-12-05T22:53:26Z

Define nameless structs for enum variants with data inline, without creating separate named Foo_Body structs, like described in #353.
Such inlining is only done for tuple variants with a single field, in other cases it's not an improvement.

Unresolved questions:

The inlining is currently done automatically and cannot be disabled. Do we need control over it? At which granularity exactly? Global config? Annotations for specific variants?

Caveats:

cbindgen doesn't currently detect field name conflicts when adding a field named tag to structs or unions generated for enums with data. This PR continues that tradition and doesn't detect conflicts when adding fields named variant_tag for inlined tags.
The inlining is not currently done when derive_tagged_enum_destructor = true or derive_tagged_enum_copy_constructor = true for the given enum. It should be possible to derive destructors and copy constructors when variant definitions are inlined, but it's annoying, e.g. array fields require special cases. I'll probably try to address this limitation next week.

Closes #353

emilio · 2020-12-06T12:40:11Z

So... it seems GCC doesn't allow to put stuff with non-trivial constructors / destructors in these unnamed structs (even when they're defined in an union, which is really odd).

I tried to implement copy-ctors and destructors in 3cd145e. It seems clang works just fine, but GCC is choking because of that restriction...

I'd love to make this the default as it is IMO a clearly-better default, but that restriction might be annoying.

petrochenkov · 2020-12-06T12:56:52Z

So... it seems GCC doesn't allow to put stuff with non-trivial constructors / destructors in these unnamed structs (even when they're defined in an union, which is really odd).

Yeah, I've seen that restriction firing on the destructor_and_copy_ctor test.
This is a good motivation for a per-variant opt-out at least (in destructor_and_copy_ctor the destructor is added manually through config, so cbindgen wouldn't be able to detect it even if it tried).

emilio · 2020-12-06T12:59:43Z

Yeah, a potential alternative is not doing this for C++ mode, since in that case you can generate the casts anyhow, which is nicer?

emilio · 2020-12-06T13:00:52Z

I thought we could work around it by using an anonymous union wrapping the field, but it seems that restriction also applies to unions for some reason? :(

emilio · 2020-12-06T13:20:13Z

(34fd6ee is the above btw)

emilio · 2020-12-06T13:25:55Z

I think I'd prefer to not do this for C++ mode at all, which would allow to always do it on C.

This is because with C++ you can derive the casts anyhow, and that both already does the right thing for single-field variants, and has the advantage that you can add assertions on them.

petrochenkov · 2020-12-07T21:49:12Z

@emilio
Do you mean entirely removing support for this from C++ mode (i.e. from derived methods), or just disabling it by default and making possible to enable it with an option?

emilio · 2020-12-07T23:09:02Z

I meant keeping C++ codegen as-is (with the inlined casts), and making C use anonymous aggregates.

It was used for adding `_` to names of numeric fields, but with inlined variants it isn't correct, so now we add `_` only if the field actually starts with a non-identifier character (a digit).

petrochenkov · 2020-12-09T17:54:42Z

Updated.

emilio

Looks great, thank you!

emilio · 2020-12-09T18:37:00Z

src/bindgen/ir/enumeration.rs

+        /// This is a reasonable thing to do only for tuple variants with a single field.
+        inline: bool,
+        /// Generated cast methods return the variant's only field instead of the variant itself.
+        /// For backward compatibility casts are inlined in a slightly


This comment might want to be tweaked.

emilio · 2020-12-09T18:37:14Z

src/bindgen/ir/enumeration.rs

@@ -27,6 +27,14 @@ pub enum VariantBody {
        name: String,
        /// The struct with all the items.
        body: Struct,
+        /// A separate named struct is not created for this variant,


Line wrapping is a bit odd here, but fine :)

* Remove artificial restriction on lifetime parameters on enums (#604) * Add an option for converting usize/isize into size_t/ptrdiff_t. (#606) * Allow controlling the cargo profile used for expansion. (#607) * Support wider range of expressions in enum discriminants (#614) * Support generation of Cython bindings (#590) * Fixed some issues with style=tag and recursive structs (#615) * Default C style to Both (as specified in docs) (#615) * Fix resolution of path dependencies from certain modules. (#629) * Support inlined definitions for tuple variants with a single field in C (#631) Thanks to all the awesome contributors that contributed to this release.

2401: Bump cbindgen from 0.15.0 to 0.16.0 r=tomtau a=dependabot-preview[bot] Bumps [cbindgen](https://github.com/eqrion/cbindgen) from 0.15.0 to 0.16.0. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/eqrion/cbindgen/blob/master/CHANGES">cbindgen's changelog</a>.</em></p> <blockquote> <h2>0.16.0</h2> <pre><code> * Remove artificial restriction on lifetime parameters on enums ([#604](mozilla/cbindgen#604)) * Add an option for converting usize/isize into size_t/ptrdiff_t. ([#606](mozilla/cbindgen#606)) * Allow controlling the cargo profile used for expansion. ([#607](mozilla/cbindgen#607)) * Support wider range of expressions in enum discriminants ([#614](mozilla/cbindgen#614)) * Support generation of Cython bindings ([#590](mozilla/cbindgen#590)) * Fixed some issues with style=tag and recursive structs ([#615](mozilla/cbindgen#615)) * Default C style to Both (as specified in docs) ([#615](mozilla/cbindgen#615)) * Fix resolution of path dependencies from certain modules. ([#629](mozilla/cbindgen#629)) * Support inlined definitions for tuple variants with a single field in C ([#631](mozilla/cbindgen#631)) </code></pre> <p>Thanks to all the awesome contributors that contributed to this release.</p> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/eqrion/cbindgen/commit/a00b4215a907601680f6e9acaf93df1cbafa8ded"><code>a00b421</code></a> v0.16.0</li> <li><a href="https://github.com/eqrion/cbindgen/commit/b82e37525478d7d0bddbea2baf27e2eb9434531f"><code>b82e375</code></a> enum: Support inlined definitions for tuple variants with a single field</li> <li><a href="https://github.com/eqrion/cbindgen/commit/eaf3e57e743dba7621bfa59d0c5bf0656a79cb71"><code>eaf3e57</code></a> enum: Remove some redundant function parameters</li> <li><a href="https://github.com/eqrion/cbindgen/commit/0083c43e13c384787023c1fc8c491744d9f41070"><code>0083c43</code></a> Remove <code>Struct::tuple_struct</code></li> <li><a href="https://github.com/eqrion/cbindgen/commit/8a5db0baebfd45f4c43681151d111cdcb31bc6c5"><code>8a5db0b</code></a> Minor cleanup to <code>fn close_brace</code></li> <li><a href="https://github.com/eqrion/cbindgen/commit/1963f0c92e93456359713db353aa6276f6200f7b"><code>1963f0c</code></a> Partially support <code>#[cfg]</code>s on fields</li> <li><a href="https://github.com/eqrion/cbindgen/commit/dfcee869ba536e661a0c972b5407ea3a5579d960"><code>dfcee86</code></a> enum: Do not forget to rename entities in enum discriminants</li> <li><a href="https://github.com/eqrion/cbindgen/commit/fbc2237b7d7b8ab250ec184c709bc5d4129fceab"><code>fbc2237</code></a> parser: Fix resolution of #[path] dependencies from certain modules.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/9f558e30f3313f2fe6fcdc172b6f86c619564414"><code>9f558e3</code></a> enum: <code>enum_name</code> -> <code>tag_name</code></li> <li><a href="https://github.com/eqrion/cbindgen/commit/8997277cb71922b028a003349c59bd87ae0afe4d"><code>8997277</code></a> enum: Break up <code>Enum::write</code> into multiple functions</li> <li>Additional commits viewable in <a href="https://github.com/eqrion/cbindgen/compare/v0.15.0...v0.16.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=cbindgen&package-manager=cargo&previous-version=0.15.0&new-version=0.16.0)](https://dependabot.com/compatibility-score/?dependency-name=cbindgen&package-manager=cargo&previous-version=0.15.0&new-version=0.16.0) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language - `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language - `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language - `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language - `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com): - Update frequency (including time of day and day of week) - Pull request limits (per update run and/or open at any time) - Out-of-range updates (receive only lockfile updates, if desired) - Security updates (receive only security updates, if desired) </details> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

Bump cbindgen from 0.14.6 to 0.20.0 Bumps [cbindgen](https://github.com/eqrion/cbindgen) from 0.14.6 to 0.20.0. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/eqrion/cbindgen/releases">cbindgen's releases</a>.</em></p> <blockquote> <h2>v0.20.0</h2> <ul> <li>Add Builder::with_using_namespaces. (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/688">#688</a>)</li> <li>Ignore PhantomPinned. (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/695">#695</a>)</li> <li>Simplify Pin to T. (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/697">#697</a>)</li> <li>Update --pretty=expanded to -Zunpretty=expanded. (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/706">#706</a>)</li> </ul> <h2>v0.19.0</h2> <ul> <li>Simplify types in generics (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/663">#663</a>)</li> <li>Use --profile=check for macro expansion (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/671">#671</a>)</li> <li>Use exported name to prefix enum variants (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/672">#672</a>)</li> <li>Fix path attribute handling in inline submodules (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/679">#679</a>)</li> <li>Fix a stack overflow with some recursive typedefs (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/680">#680</a>)</li> </ul> <h2>v0.18.0</h2> <ul> <li>Simplify types in nested types such as pointed-to types and function signatures (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/661">#661</a>)</li> </ul> <h2>v0.17.0</h2> <ul> <li>Add with_parse_extra_bindings to builder. (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/645">#645</a>)</li> <li>Support NonZero and fix incorrect simplification of Option into ptr. (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/647">#647</a>)</li> <li>Deal with name conflicts correctly in declaration type resolution. (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/651">#651</a>)</li> <li>Support pointers to ZSTs. (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/656">#656</a>)</li> </ul> <h2>v0.16.0</h2> <p>No release notes provided.</p> <h2>v0.15.0</h2> <p>No release notes provided.</p> </blockquote> </details> <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/eqrion/cbindgen/blob/master/CHANGES">cbindgen's changelog</a>.</em></p> <blockquote> <h2>0.20.0</h2> <pre><code> * Add Builder::with_using_namespaces. ([#688](mozilla/cbindgen#688)) * Ignore PhantomPinned. ([#695](mozilla/cbindgen#695)) * Simplify Pin<T> to T. ([#697](mozilla/cbindgen#697)) * Update --pretty=expanded to -Zunpretty=expanded. ([#706](mozilla/cbindgen#706)) </code></pre> <h2>0.19.0</h2> <pre><code> * Simplify types in generics ([#663](mozilla/cbindgen#663)) * Use --profile=check for macro expansion ([#671](mozilla/cbindgen#671)) * Use exported name to prefix enum variants ([#672](mozilla/cbindgen#672)) * Fix path attribute handling in inline submodules ([#679](mozilla/cbindgen#679)) * Fix a stack overflow with some recursive typedefs ([#680](mozilla/cbindgen#680)) </code></pre> <h2>0.18.0</h2> <pre><code> * Simplify types in nested types such as pointed-to types and function signatures ([#661](mozilla/cbindgen#661)) </code></pre> <h2>0.17.0</h2> <pre><code> * Add with_parse_extra_bindings to builder. ([#645](mozilla/cbindgen#645)) * Support NonZero and fix incorrect simplification of Option<ptr> into ptr. ([#647](mozilla/cbindgen#647)) * Deal with name conflicts correctly in declaration type resolution. ([#651](mozilla/cbindgen#651)) * Support pointers to ZSTs. ([#656](mozilla/cbindgen#656)) </code></pre> <h2>0.16.0</h2> <pre><code> * Remove artificial restriction on lifetime parameters on enums ([#604](mozilla/cbindgen#604)) * Add an option for converting usize/isize into size_t/ptrdiff_t. ([#606](mozilla/cbindgen#606)) * Allow controlling the cargo profile used for expansion. ([#607](mozilla/cbindgen#607)) * Support wider range of expressions in enum discriminants ([#614](mozilla/cbindgen#614)) * Support generation of Cython bindings ([#590](mozilla/cbindgen#590)) * Fixed some issues with style=tag and recursive structs ([#615](mozilla/cbindgen#615)) * Default C style to Both (as specified in docs) ([#615](mozilla/cbindgen#615)) * Fix resolution of path dependencies from certain modules. ([#629](mozilla/cbindgen#629)) * Support inlined definitions for tuple variants with a single field in C ([#631](mozilla/cbindgen#631)) </code></pre> <p>Thanks to all the awesome contributors that contributed to this release.</p> <h2>0.15.0</h2> <pre><code> * Allow customizing mangling of generic parameters in C ([#575](mozilla/cbindgen#575)) * Box<T> simplifies to T* in C (4ce324c) * ManuallyDrop<T> and MaybeUninit<T> simplify to T in C, and are opaque in C++ (0076a17) * C++ supports a derive-ostream annotation to derive serialization of structs, unions and enums ([#582](mozilla/cbindgen#582)) * Large character constants have been fixed on Windows ([#586](mozilla/cbindgen#586)) * Constants are now generated for typedefs, etc ([#589](mozilla/cbindgen#589)) * The `sort_by` configuration option has been made to work for constants ([#587](mozilla/cbindgen#587)) * Default sort order is source order now (sort_by = "None"), and can be changed by the above option ([#587](mozilla/cbindgen#587)) </code></pre> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/eqrion/cbindgen/commit/41506d5aeb2be8d200f52d08afb0c243414eb00d"><code>41506d5</code></a> v0.20.0</li> <li><a href="https://github.com/eqrion/cbindgen/commit/34299aef5610fa82a058374bdfce8d782e0abe29"><code>34299ae</code></a> Don't use <code>check</code> profile when expanding code on a release build</li> <li><a href="https://github.com/eqrion/cbindgen/commit/6c96c8ab957542eb0bebe576fee3fb1241db0114"><code>6c96c8a</code></a> Update --pretty=expanded to -Zunpretty=expanded</li> <li><a href="https://github.com/eqrion/cbindgen/commit/57add9c86083b492266c8511fba30fb7c37cce25"><code>57add9c</code></a> Fix some clippy lints.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/63c1043dfb92fcac0bf3073c2d71e9e4ac69c943"><code>63c1043</code></a> Simplify Pin<T> to T</li> <li><a href="https://github.com/eqrion/cbindgen/commit/ccd1f0e9ec2369735b9481c85057b9c65b6b0908"><code>ccd1f0e</code></a> add <code>Builder::with_using_namespaces</code></li> <li><a href="https://github.com/eqrion/cbindgen/commit/4e394493d63c2348a08d486562dc2082cbdfe306"><code>4e39449</code></a> Ignore PhantomPinned</li> <li><a href="https://github.com/eqrion/cbindgen/commit/2d20c4b15179523b10605bdd95006882dd1c8e4e"><code>2d20c4b</code></a> Move the target-guessing code from <a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/676">#676</a> to its own function.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/93c06c5c9d319f481788c9670700097b4e46d270"><code>93c06c5</code></a> Only fetch dependencies for current platform by default (<a href="https://github-redirect.dependabot.com/eqrion/cbindgen/issues/676">#676</a>)</li> <li><a href="https://github.com/eqrion/cbindgen/commit/d9e490ce8b836194595bd30611253a7028059da2"><code>d9e490c</code></a> v0.19.0</li> <li>Additional commits viewable in <a href="https://github.com/eqrion/cbindgen/compare/v0.14.6...v0.20.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=cbindgen&package-manager=cargo&previous-version=0.14.6&new-version=0.20.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details>

v0.16.0 * Remove artificial restriction on lifetime parameters on enums (mozilla#604) * Add an option for converting usize/isize into size_t/ptrdiff_t. (mozilla#606) * Allow controlling the cargo profile used for expansion. (mozilla#607) * Support wider range of expressions in enum discriminants (mozilla#614) * Support generation of Cython bindings (mozilla#590) * Fixed some issues with style=tag and recursive structs (mozilla#615) * Default C style to Both (as specified in docs) (mozilla#615) * Fix resolution of path dependencies from certain modules. (mozilla#629) * Support inlined definitions for tuple variants with a single field in C (mozilla#631) Thanks to all the awesome contributors that contributed to this release. # Conflicts: # src/bindgen/ir/enumeration.rs # src/bindgen/writer.rs

petrochenkov mentioned this pull request Dec 5, 2020

Add a feature to simplify enums used as tagged unions #353

Closed

petrochenkov added 4 commits December 9, 2020 20:51

Minor cleanup to fn close_brace

573e787

Remove Struct::tuple_struct

d48b7ff

It was used for adding `_` to names of numeric fields, but with inlined variants it isn't correct, so now we add `_` only if the field actually starts with a non-identifier character (a digit).

enum: Remove some redundant function parameters

0e0eae7

enum: Support inlined definitions for tuple variants with a single field

2b1bae1

petrochenkov force-pushed the enum3 branch from fb6e1b9 to 2b1bae1 Compare December 9, 2020 17:54

emilio approved these changes Dec 9, 2020

View reviewed changes

emilio merged commit b82e375 into mozilla:master Dec 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enum: Support inlined definitions for tuple variants with a single field #631

enum: Support inlined definitions for tuple variants with a single field #631

petrochenkov commented Dec 5, 2020

emilio commented Dec 6, 2020

petrochenkov commented Dec 6, 2020

emilio commented Dec 6, 2020

emilio commented Dec 6, 2020

emilio commented Dec 6, 2020

emilio commented Dec 6, 2020

petrochenkov commented Dec 7, 2020

emilio commented Dec 7, 2020

petrochenkov commented Dec 9, 2020

emilio left a comment

emilio Dec 9, 2020

emilio Dec 9, 2020

enum: Support inlined definitions for tuple variants with a single field #631

enum: Support inlined definitions for tuple variants with a single field #631

Conversation

petrochenkov commented Dec 5, 2020

Unresolved questions:

Caveats:

emilio commented Dec 6, 2020

petrochenkov commented Dec 6, 2020

emilio commented Dec 6, 2020

emilio commented Dec 6, 2020

emilio commented Dec 6, 2020

emilio commented Dec 6, 2020

petrochenkov commented Dec 7, 2020

emilio commented Dec 7, 2020

petrochenkov commented Dec 9, 2020

emilio left a comment

Choose a reason for hiding this comment

emilio Dec 9, 2020

Choose a reason for hiding this comment

emilio Dec 9, 2020

Choose a reason for hiding this comment