Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
add a stucts-and-tuples chapter #31
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
add a stucts-and-tuples chapter #31
Changes from 17 commits
c3a54f9
08062bd
eb951c2
d5a144b
569338b
d96723f
4974192
89a8a70
c7d728e
65f65f9
fbafc2d
fbf35bf
97622bc
b0c86ea
f146fc4
72f5db3
17dab33
a9223e2
ece91f5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
As @rkruppe noted on Zulip, this seems like a problem. It means that "copy-pasting
struct
definitions and addingrepr(C)
everywhere" does not give you C compatibility, because yourFoo
would actually take space when put in a larger struct inC
.This seems like a bug, TBH. I am not sure if it is a bug that we can still fix. Might be worth having at least a warning.
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.
Yes — I was thinking the same in that conversation. That is, "bug and not clearly a bug we can fix", which does suggest that at least a lint is warranted.
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.
For the benefit of others, the Zulip conversation was here.
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.
That's not actually how C works - C does not allow empty structs. The godbolt link was from C++, which does allow empty structs, and in which empty structs have size one. Basically, this would only be an issue for somebody like Mozilla, who talk to C++ through a non-C ABI.
Notably, also, gcc and clang's C extension for empty structs has
sizeof(struct { }) = 0
: https://godbolt.org/z/K3_5fJThere 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.
Transcribing another thing @ubsan noted on Zulip: empty structs are accepted as an extension by some C compilers, but (at least) GCC and Clang make them have size zero, unlike C++. Example: https://godbolt.org/z/AS2gdC
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.
Allow-by-default at best - 0 size structures are weird in C++, and usually you'd use either EBO or
[[no_unique_address]]
with them.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.
Isn't them being weird another argument for making this a warn-by-default lint? I expect many people will not know this. I am not a C++ expert, but I have programmed in C++ for many years and never heard about this; I don't think we can expect everybody doing C++ FFI to know about these issues.
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 to summarize: C does not allow empty structs, some C language extensions allow empty structs with
sizeof == 0
, C++ does allow empty structs but these have asizeof == 1
unless they are inherited from or they are fields that have the[[no_unique_address]]
attribute (in both cases, they don't increase the size of the struct - i'm unsure what role the alignment of the type plays though).I think
#[repr(C)]
should warn-by-default on this when it makes a difference, that is, when the ZST would change the layout. We could have an opt-in warning that always warns on ZST being used in#[repr(C)]
but I fear that would be extremely noisy for little win.About the situation with the C-language extension and C++ it appears that
#[repr(C)] != #[repr(Cxx)]
, so maybe we just need to add newrepr
s to deal with those. In the mean time it might be worth it to just ignore C++ while specifying#[repr(C)]
here (maybe add a note so that we don't forget).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 had to google EBO: Empty base optimization.
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.
@RalfJung note that EBO is guaranteed by the C++>=11 standard for types with standard layout, like empty structs:
struct T {};
, so it is a required layout optimization.