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.

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 note for array with empty struct or union #365

Merged
merged 1 commit into from
May 17, 2023
Merged

Add note for array with empty struct or union #365

merged 1 commit into from
May 17, 2023

Conversation

kito-cheng
Copy link
Collaborator

Fix #358

The key point of this issue is C and C++ having different type size for array of empty struct or union, size of empty struct or union is 0 for C, and 1 for C++, and the size of array is type size multiplied by the length of the array.

Also checked AArch64 code gen, they also generated different code between C and C++, and ABI isn't explicitly describe this but has describe the size of array is type * length.

@asb
Copy link
Collaborator

asb commented Feb 10, 2023

Thanks for looking at this Kito. Quick high level comment: I think the case of a zero-length array of empty structs/unions should also be mentioned. Perhaps it's worth being more explicit about the case of a 1-sized array of an empty struct/union too?

@kito-cheng kito-cheng force-pushed the fix-358 branch 2 times, most recently from 1ea95a3 to 950849a Compare February 16, 2023 16:01
@kito-cheng
Copy link
Collaborator Author

Changes:

  • Add example to show the case struct { struct {} e[1]; float f; };
  • Add sentence to mention zero-length array will ignored during struct flatten.

riscv-cc.adoc Outdated Show resolved Hide resolved
riscv-cc.adoc Outdated Show resolved Hide resolved
riscv-cc.adoc Outdated Show resolved Hide resolved
riscv-cc.adoc Outdated Show resolved Hide resolved
riscv-cc.adoc Outdated Show resolved Hide resolved
@kito-cheng
Copy link
Collaborator Author

@nick-knight thanks for your comment and suggestion :)

@kito-cheng
Copy link
Collaborator Author

@asb @jrtc27 @luismarques did you mind take a review for that? I would like to have your review before I merge that :)

@luismarques luismarques self-requested a review March 14, 2023 16:44
@luismarques
Copy link
Collaborator

luismarques commented Mar 14, 2023

Sorry, I shouldn't have approved this. What is there LGTM but it seems it doesn't explicitly cover all cases that @asb was asking about. Can you be more explicit about both the [0] and the [1] cases?

@kito-cheng kito-cheng force-pushed the fix-358 branch 2 times, most recently from 17e7bf4 to ffead46 Compare March 23, 2023 10:08
@kito-cheng
Copy link
Collaborator Author

@luismarques added more description for zero-length array of empty structs/union, thank for the review :)

@kito-cheng
Copy link
Collaborator Author

Changes:

  • Explicitly say the field with array of empty struct type will be ignored or not in the example.

@asb
Copy link
Collaborator

asb commented Mar 30, 2023

@kito-cheng the last round of grammatical suggestions from @nick-knight are marked as resolved but the changes don't seem to be reflected in the current version of the patch. It looks like they might have got lot along the way?

I think I may have some additional minor grammatical or phrasing tweak suggestions after that, but it might be inefficient to comment if the version of the patch in this PR is an older version.

@kito-cheng
Copy link
Collaborator Author

@asb thanks for catch that...I must did something wrong during update this PR, updated now :)

Copy link
Collaborator

@asb asb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo the minor wording fixes suggested in line. Thanks Kito!

riscv-cc.adoc Outdated Show resolved Hide resolved
riscv-cc.adoc Outdated Show resolved Hide resolved
Fix #358

The key point of this issue is C and C++ having different type size for
array of empty struct or union, size of empty struct or union is 0 for
C, and 1 for C++, and the size of array is type size multiplied by the
length of the array.

Also checked AArch64 code gen, they also generated different code between C
and C++, and ABI isn't explicitly describe this but has describe the size
of array is type * length.
@kito-cheng
Copy link
Collaborator Author

Thanks everyone :)

@kito-cheng kito-cheng deleted the fix-358 branch May 17, 2023 13:24
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.

Clarification of rules for flattening structs containing arrays of empty records
4 participants