-
Notifications
You must be signed in to change notification settings - Fork 307
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
ir: Add an option to generate an empty, default constructor for tagged enums. #377
Conversation
r? @gankro |
# Whether enums with fields should generate an empty, private destructor. | ||
# This allows the auto-generated constructor functions to compile, if there are | ||
# non-trivially constructible members. This falls in the same family of | ||
# dangerousness as `derive_tagged_enum_copy_constructor` and co. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm completely unfamiliar with the issue and having trouble making sense of it, can you expand on this? what types don't we support? i'm not familiar with the concept of "trivially constructible".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the issue is the following. Simplified.
This builds, as expected:
#include <cstdint>
#include <new>
struct TypeWithDestructor {
uint8_t field;
// ~TypeWithDestructor() {}
};
struct TaggedUnion {
static TaggedUnion Create(const TypeWithDestructor& a) {
TaggedUnion ret;
ret.type = 0;
new (&ret.foo) TypeWithDestructor(a);
return ret;
}
uint8_t type;
union {
TypeWithDestructor foo;
};
};
If you uncomment the destructor, then it doesn't, because C++ will implicitly delete the default constructor of TaggedUnion
.
Since all we want is a default constructor that doesn't initialize anything, so that Create()
does the right thing, a private default constructor serves this purpose. It also avoids people accidentally creating uninitialized structs themselves, which is a win-win.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The final simplified example for cbindgen would be:
#include <cstdint>
#include <new>
struct TypeWithDestructor {
uint8_t field;
~TypeWithDestructor() {}
};
struct TaggedUnion {
static TaggedUnion Create(const TypeWithDestructor& a) {
TaggedUnion ret;
ret.type = 0;
new (&ret.foo) TypeWithDestructor(a);
return ret;
}
private:
TaggedUnion() {}
public:
~TaggedUnion() { /* cbindgen-generated */ }
uint8_t type;
union {
TypeWithDestructor foo;
};
};
With nicer enum types and such of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the claim that this is dangerous just incorrect? Adding a private default constructor doesn't actually change the ABI in any meaningful way, does it? It just enables you to add destructors that do mess up the ABI..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding constructors changes the ABI of return types in MSVC: https://docs.microsoft.com/en-us/cpp/build/x64-calling-convention?view=vs-2019#return-values
To return a user-defined type by value in RAX, it must have a length of 1, 2, 4, 8, 16, 32, or 64 bits. It must also have no user-defined constructor, destructor, or copy assignment operator; no private or protected non-static data members; no non-static data members of reference type; no base classes; no virtual functions; and no data members that do not also meet these requirements. (This is essentially the definition of a C++03 POD type. Because the definition has changed in the C++11 standard, we don't recommend using std::is_pod for this test.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yeah I wish I didn't know this :)
d8745f0
to
3f1516a
Compare
I sent mozilla/cbindgen#377 since I got sick of copy-pasting the private default constructor stuff :) Differential Revision: https://phabricator.services.mozilla.com/D41419 --HG-- extra : moz-landing-system : lando
I sent mozilla/cbindgen#377 since I got sick of copy-pasting the private default constructor stuff :) Differential Revision: https://phabricator.services.mozilla.com/D41419 --HG-- extra : moz-landing-system : lando
I sent mozilla/cbindgen#377 since I got sick of copy-pasting the private default constructor stuff :) Differential Revision: https://phabricator.services.mozilla.com/D41419
I sent mozilla/cbindgen#377 since I got sick of copy-pasting the private default constructor stuff :) Differential Revision: https://phabricator.services.mozilla.com/D41419
I sent mozilla/cbindgen#377 since I got sick of copy-pasting the private default constructor stuff :) Differential Revision: https://phabricator.services.mozilla.com/D41419
@Gankra anything blocking this? Thanks! |
…d enums. This allows to clean up a pattern that has been showing up lately, see occurrences of: https://searchfox.org/mozilla-central/rev/9775cca0a10a9b5c5f4e15c8f7b3eff5bf91bbd0/servo/ports/geckolib/cbindgen.toml#329
55f1f1c
to
a089ffb
Compare
FRIG i swear i keep hallucinating that i said yes to this pls ship it |
This cleans up the pattern of "Use a private dtor so that the helper functions do the right thing" by enabling it everywhere using: mozilla/cbindgen#377 It also caught some uninitialized value issues. I think they're mostly harmless since we zero-initialize our structs: https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/servo/components/style/properties/gecko.mako.rs#632 And since we override the clip rect, which is the other bit of code that was failing to build with this change. Differential Revision: https://phabricator.services.mozilla.com/D43491 --HG-- extra : moz-landing-system : lando
This cleans up the pattern of "Use a private dtor so that the helper functions do the right thing" by enabling it everywhere using: mozilla/cbindgen#377 It also caught some uninitialized value issues. I think they're mostly harmless since we zero-initialize our structs: https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/servo/components/style/properties/gecko.mako.rs#632 And since we override the clip rect, which is the other bit of code that was failing to build with this change. Differential Revision: https://phabricator.services.mozilla.com/D43491
Changelog: ## 0.9.1 * Various improvements to comment output. mozilla/cbindgen#370 / mozilla/cbindgen#375. * Fixed expand when ran from build.rs. mozilla/cbindgen#371 * More debugging output for expansion. mozilla/cbindgen#383 * New option to add a default private constructor in C++ tagged enums. mozilla/cbindgen#377 * Syn and related dependencies updated to 1.0. mozilla/cbindgen#379
This cleans up the pattern of "Use a private dtor so that the helper functions do the right thing" by enabling it everywhere using: mozilla/cbindgen#377 It also caught some uninitialized value issues. I think they're mostly harmless since we zero-initialize our structs: https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/servo/components/style/properties/gecko.mako.rs#632 And since we override the clip rect, which is the other bit of code that was failing to build with this change. Differential Revision: https://phabricator.services.mozilla.com/D43491
I sent mozilla/cbindgen#377 since I got sick of copy-pasting the private default constructor stuff :) Differential Revision: https://phabricator.services.mozilla.com/D41419 UltraBlame original commit: 849ca4b3f9de609933014881329f51dc897e13b5
I sent mozilla/cbindgen#377 since I got sick of copy-pasting the private default constructor stuff :) Differential Revision: https://phabricator.services.mozilla.com/D41419 UltraBlame original commit: f252db10f2e09c75ea4f2db63e101fd53b8e7b4c
This cleans up the pattern of "Use a private dtor so that the helper functions do the right thing" by enabling it everywhere using: mozilla/cbindgen#377 It also caught some uninitialized value issues. I think they're mostly harmless since we zero-initialize our structs: https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/servo/components/style/properties/gecko.mako.rs#632 And since we override the clip rect, which is the other bit of code that was failing to build with this change. Differential Revision: https://phabricator.services.mozilla.com/D43491 UltraBlame original commit: 6c3f50c6f6662291139bc292fb9dd77aa64331ee
I sent mozilla/cbindgen#377 since I got sick of copy-pasting the private default constructor stuff :) Differential Revision: https://phabricator.services.mozilla.com/D41419 UltraBlame original commit: 849ca4b3f9de609933014881329f51dc897e13b5
I sent mozilla/cbindgen#377 since I got sick of copy-pasting the private default constructor stuff :) Differential Revision: https://phabricator.services.mozilla.com/D41419 UltraBlame original commit: f252db10f2e09c75ea4f2db63e101fd53b8e7b4c
I sent mozilla/cbindgen#377 since I got sick of copy-pasting the private default constructor stuff :) Differential Revision: https://phabricator.services.mozilla.com/D41419 UltraBlame original commit: 849ca4b3f9de609933014881329f51dc897e13b5
I sent mozilla/cbindgen#377 since I got sick of copy-pasting the private default constructor stuff :) Differential Revision: https://phabricator.services.mozilla.com/D41419 UltraBlame original commit: f252db10f2e09c75ea4f2db63e101fd53b8e7b4c
This cleans up the pattern of "Use a private dtor so that the helper functions do the right thing" by enabling it everywhere using: mozilla/cbindgen#377 It also caught some uninitialized value issues. I think they're mostly harmless since we zero-initialize our structs: https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/servo/components/style/properties/gecko.mako.rs#632 And since we override the clip rect, which is the other bit of code that was failing to build with this change. Differential Revision: https://phabricator.services.mozilla.com/D43491 UltraBlame original commit: 6c3f50c6f6662291139bc292fb9dd77aa64331ee
This cleans up the pattern of "Use a private dtor so that the helper functions do the right thing" by enabling it everywhere using: mozilla/cbindgen#377 It also caught some uninitialized value issues. I think they're mostly harmless since we zero-initialize our structs: https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/servo/components/style/properties/gecko.mako.rs#632 And since we override the clip rect, which is the other bit of code that was failing to build with this change. Differential Revision: https://phabricator.services.mozilla.com/D43491 UltraBlame original commit: 6c3f50c6f6662291139bc292fb9dd77aa64331ee
This allows to clean up a pattern that has been showing up lately, see
occurrences of:
https://searchfox.org/mozilla-central/rev/9775cca0a10a9b5c5f4e15c8f7b3eff5bf91bbd0/servo/ports/geckolib/cbindgen.toml#329