Xiangling_L marked an inline comment 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: > Xiangling_L wrote: > > 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. > Okay, the other option I'm open is setting > `HandledFirstNonOverlappingEmptyField` to `true` up front when not dealing > with AIX `power` alignment. Thanks, that works too. I will address it in the next commit. 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
