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.

Sign up for GitHub

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

Jump to bottom

Fix movability of structs with enum fields #1179

Merged
merged 5 commits into from
Nov 3, 2022
Merged

Fix movability of structs with enum fields #1179

merged 5 commits into from
Nov 3, 2022

Conversation

adetaylor
Copy link
Collaborator

@adetaylor adetaylor commented Oct 23, 2022 •
edited

Fixes #1170.

@adetaylor adetaylor changed the title Adding failing test for #1170. Fix movability of structs with enum fields Oct 25, 2022
@adetaylor
Copy link
Collaborator Author

This is failing test_typedef_to_ns_enum with:

cargo:warning=In file included from /var/folders/9l/cjqbc3ws3bxcjh6h2rykj90800m02d/T/.tmpR5pSvn/target/cxx/gen0.cxx:2:
cargo:warning=/var/folders/9l/cjqbc3ws3bxcjh6h2rykj90800m02d/T/.tmpR5pSvn/target/include/autocxxgen_ffi.h:44:93: error: 'b' does not refer to a type name in pseudo-destructor expression; expected the name of type 'a::b'
cargo:warning=inline void b_synthetic_destructor_0xca48833b4f146bc9_autocxx_wrapper(a::b* arg0)  { arg0->~b(); }

@adetaylor
Simpler test for #1170.
04eab32
@adetaylor
Properly propagate deps for non-POD structs
4eab29a 
Bug #1170 is a bug where a type wasn't deemed movable, and because it
appeared in a vector, that meant misgeneration of code.

Movability calculations are done based on the presence of move
constructors for each field type. This analysis is done in a
depth-first fashion, to ensure that fields of fields are calculated
first. However, this depth-first analysis wasn't working correctly
because we weren't considering all the links in that tree.
Specifically, a type A was considered to depend on its field type
B only for POD structs in the past. It wasn't recognized that
such dependency relationships were required even for a non-POD type A,
in order that this movability/destroyability calculation could
be done correctly for type A.

This change ensures that A always depends on B irrespective of
whether A is POD or non-POD.

This is a fairly significant change to our dependency relationships.
@adetaylor
Consider that enums are always movable/destroyable.
29296ee 
The other root cause behind bug #1170 was that C++ enums did not
get move constructors nor destructors generated (they don't need them!)
- but their absence was being used as evidence that any outer type
containing such a field was also not movable.

For example,

  struct C {
    enum D {} some_enum_field;
  };

C would not be considered movable, and therefore could not exist
in a C++ vector.

This change arranges to ensure that enums are always considered
movable or destroyable in this analysis.
@adetaylor adetaylor merged commit 59da591 into main Nov 3, 2022
@adetaylor adetaylor mentioned this pull request Nov 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Trait bound error from autocxx bindings
1 participant
@adetaylor