Xiangling_L marked 9 inline comments as done.
Xiangling_L added inline comments.

Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1796
+  bool FoundFirstNonOverlappingEmptyFieldToHandle =
+      DefaultsToAIXPowerAlignment && FieldOffset == CharUnits::Zero() &&
+      !HandledFirstNonOverlappingEmptyField && !IsOverlappingEmptyField;
hubert.reinterpretcast wrote:
> The condition is still more complex than I think it should be.
> If we have found a "first" other-than-overlapping-empty-field, then we should 
> set `HandledFirstNonOverlappingEmptyField` to `true` for non-union cases.
> If `HandledFirstNonOverlappingEmptyField` being `false` is not enough for 
> `FieldOffset == CharUnits::Zero()` to be true, then I think the correction 
> would be to set `HandledFirstNonOverlappingEmptyField` in more places.
> I would like to remove the check on `FieldOffset == CharUnits::Zero()` from 
> here and instead have an assertion that 
> `!HandledFirstNonOverlappingEmptyField` implies `FieldOffset == 
> CharUnits::Zero()`.
> Also, since we're managing `HandledFirstNonOverlappingEmptyField` in non-AIX 
> cases, we should remove the `DefaultsToAIXPowerAlignment` condition for what 
> is currently named `FoundFirstNonOverlappingEmptyFieldToHandle` (adjusting 
> uses of it as necessary) and rename 
> `FoundFirstNonOverlappingEmptyFieldToHandle` to 
> `FoundFirstNonOverlappingEmptyField`.
> Also, since we're managing HandledFirstNonOverlappingEmptyField in non-AIX 
> cases, we should remove the DefaultsToAIXPowerAlignment condition for what is 
> currently named FoundFirstNonOverlappingEmptyFieldToHandle 

I am not sure if we want to remove the `DefaultsToAIXPowerAlignment` condition 
and bother with maintaining correct status of 
`HandledFirstNonOverlappingEmptyField` for other targets.

We are actually claiming `HandledFirstNonOverlappingEmptyField` is an auxiliary 
flag used for AIX only in its definition comments.

Besides, if we do want to manage `HandledFirstNonOverlappingEmptyField` in 
non-AIX cases, I noticed that we have to set this flag to `true` somewhere for 
objective-C++ cases. 

Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1834
+    TypeInfo TI = Context.getTypeInfo(D->getType());
+    FieldAlign = Context.toCharUnitsFromBits(TI.Align);
+    AlignIsRequired = TI.AlignIsRequired;
hubert.reinterpretcast wrote:
> I guess this works (we have a test for it), but the previous code made a 
> point to use the element type and not the array type (and the comment above 
> says we can't directly query `getTypeInfo` with the array type). 
> @Xiangling_L, can you confirm if the comment is out-of-date and update it?
I am sure `getTypeInfo` can recognize the element type for `IncompleteArray`. I 
will update the comments.



cfe-commits mailing list

Reply via email to