Repository navigation
fix(folding): correct unique object rep checks [GH #327] - #326
jakub-labenski wants to merge 1 commit into
Conversation
| A base class that declares no nonstatic data members of its own but inherits | ||
| some (and is therefore not empty) was treated as an empty base class. Its | ||
| size was then counted as tail padding, so the trait was incorrectly FALSE for | ||
| classes derived from it. For example, with --g++: |
There was a problem hiding this comment.
The issue is not actually gcc specific, but also applies to default/standard mode, so should just have an example with --c++11.
There was a problem hiding this comment.
Entry has been updated: --g++ was change to --c++11 and an empty message was added to static_assert.
| This is now fixed. In addition, the following version-dependent behaviors | ||
| of g++ and Clang are now emulated: | ||
|
|
||
| - An unnamed bit-field of nonzero width is treated as padding (making the |
There was a problem hiding this comment.
What should the behavior for standard-mode be in this case?
There was a problem hiding this comment.
In standard mode, an unnamed bit-field of nonzero width is treated as padding, so __has_unique_object_representations(S) evaluates to false.
This was fixed in the latest update, along with new tests covering standard mode as well as GCC, Clang, and Microsoft emulation modes.
| //remark:has_unique_object_representations with a base that has no direct fields | ||
| //type:fp | ||
| //options_all:--c++17 | ||
| //options:--g++ --gnu_version=150100:--clang --clang_version=190000 |
There was a problem hiding this comment.
The test case should also test standard mode (and ideally, also MSVC modes).
Also need to have test coverage for the other code changes (unnamed bit-field, volatile qualifiers). Should also test each version condition from the source code.
There was a problem hiding this comment.
I've updated the tests to include standard mode and Microsoft emulation, and added the missing tests for the remaining changes.
| field->source_corresp.assoc_info == | ||
| (char *)get_unnamed_field_symbol()) { | ||
| /* An unnamed bit-field of non-zero width is padding. */ | ||
| result = FALSE; |
There was a problem hiding this comment.
Does that need to be restricted to non-union class types?
There was a problem hiding this comment.
Yes, it should. In a union, an unnamed bit-field overlaps the other members, so it shouldn't be treated as padding on its own. Instead, any bit-field narrower than the union is treated as padding, except in Clang and Microsoft modes (which ignore that case). This is now implemented and tests have been added.
c05bcb2 to
625b0c5
Compare
|
I'm not sure what the recommended practice is for syncing with main. I rebased the branch (and the pull request) onto main, then amended my commit to keep the git history clean. Below is an AI-generated summary of the changes in my commit only:
|
jakub-labenski
changed the title
|
AcknowlEDG review results updated for
|
|
|
||
| This is now fixed. In addition: | ||
|
|
||
| - An unnamed bit-field of nonzero width is now treated as padding (making |
There was a problem hiding this comment.
Can we remove "(making the result FALSE), as the standard requires" here. I don't think it really adds much information, and the Changes entry is already quite long.
| the result FALSE), as the standard requires. For example: | ||
|
|
||
| struct S { char a; char : 8; char b; }; | ||
| // __has_unique_object_representations(S) is now FALSE. |
There was a problem hiding this comment.
Change that to a static_assert
| struct S { char a; char : 8; char b; }; | ||
| // __has_unique_object_representations(S) is now FALSE. | ||
|
|
||
| The previous behavior (TRUE) is retained in g++ modes before 13.1 |
There was a problem hiding this comment.
I think this should just be added to the previous sentence with ", except in ..."
| also does not treat it as padding, matching MSVC. For example: | ||
|
|
||
| union U { int x : 3; }; | ||
| // __has_unique_object_representations(U) is now TRUE with |
There was a problem hiding this comment.
Change to a static_assert and move the --microsoft to "For example, with --microsoft".
| not having unique object representations only in g++ modes before | ||
| version 10 (previously this was done for all g++ versions, including | ||
| Clang modes). In other modes the result is the same as for the | ||
| unqualified type, as the standard requires. |
There was a problem hiding this comment.
Maybe the second sentence can also be dropped?
| //remark:has_unique_object_representations with unnamed bit-fields | ||
| //type:fp | ||
| //options_all:--c++11 | ||
| //options:-DSTD_MODE:--g++ --gnu_version=90400 -DOLD_PAD:--g++ --gnu_version=100400 -DOLD_PAD:--g++ --gnu_version=100500:--g++ --gnu_version=120200 -DOLD_PAD:--g++ --gnu_version=120300:--g++ --gnu_version=150100:--clang --clang_version=160000 -DOLD_PAD:--clang --clang_version=170000:--microsoft |
There was a problem hiding this comment.
Instead of adding macro definitions (STD_MODE, OLD_PAD), can we change the tests to check the GCC/Clang version macros directly. The idea here would be that the test case can be pasted into godbolt where it then should pass with any compiler selected (without having to adjust macro definitions).
625b0c5 to
f5ecadb
Compare
|
AcknowlEDG review results updated for
CI other. |
|
AcknowlEDG review results updated for
CI success. |
daveedvdv-nvidia
left a comment
daveedvdv-nvidia
left a comment
There was a problem hiding this comment.
Thanks for taking this on!
It looks good to me. I have a tiny stylistic nit — that's all.
| field->is_bit_field && field->bit_size != 0 && | ||
| unnamed_bit_field_is_padding() && | ||
| field->source_corresp.assoc_info == | ||
| (char*)get_unnamed_field_symbol()) { |
There was a problem hiding this comment.
I'd rewrite that last bit as symbol_for(field) == get_unnamed_field_symbol)()) {
Fix __has_unique_object_representations for several cases: - A base class with no direct nonstatic data members that inherits some (and is thus not empty) was treated as an empty base. Its size was counted as tail padding, making the trait incorrectly FALSE for derived classes. Use is_empty_class_type instead of checking for an empty field list. - Treat unnamed bit-fields of nonzero width in classes as padding, as the standard requires. Keep the old behavior only in Clang modes before 17 and g++ modes before 13.1 (except the 10.5, 11.4, and 12.3 backports). - Do not apply the unnamed bit-field rule to unions, where the bit-field overlaps the other members; the existing union size check decides. - In Microsoft mode, as in Clang mode, do not treat a union bit-field narrower than the union as padding, matching MSVC. - Restrict treating volatile-qualified non-class types as lacking unique object representations to g++ modes before version 10, and no longer apply it in Clang modes. Add regression tests has-unique-object-rep-inherited-base, has-unique-object-rep-bit-field, has-unique-object-rep-union-bit-field, and has-unique-object-rep-volatile, covering standard, g++, Clang, and Microsoft modes, and update expectations for imported Clang type-traits and g++ has-unique-obj-representations3 tests.
f5ecadb to
4b5fe66
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly identifies a folding fix for unique object representation checks, which matches the primary changes and regression tests. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 4… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 4 files. (29 skipped: 28 unsupported, 1 too large.)
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Comment @coderabbitai help to get the list of available commands.
|
AcknowlEDG review results updated for
CI other. |
|
Thanks @daveedvdv-nvidia and @nv-cmeerw for the review. |
This branch has not been deployed
4 participants
Discussion: #290
Fix __has_unique_object_representations for several cases:
A base class with no direct nonstatic data members that inherits some (and is thus not empty) was treated as an empty base. Its size was counted as tail padding, making the trait incorrectly FALSE for derived classes. Use is_empty_class_type instead of checking for an empty field list.
Treat unnamed bit-fields of nonzero width as padding in Clang 17+ and g++ 13.1+ (also g++ 10.5, 11.4, 12.3 backports).
Restrict treating volatile-qualified non-class types as lacking unique object representations to g++ modes before version 10, and no longer apply it in Clang modes.
Add regression test has-unique-object-rep-inherited-base and update expectations for imported Clang type-traits and g++ has-unique-obj-representations3 tests.
Summary by CodeRabbit
__has_unique_object_representationsresults for types with inherited data members, unnamed bit-fields, and union bit-field padding.