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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79719/new/ https://reviews.llvm.org/D79719 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
