hubert.reinterpretcast added inline comments.
================ Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1796 + bool FoundFirstNonOverlappingEmptyFieldToHandle = + DefaultsToAIXPowerAlignment && FieldOffset == CharUnits::Zero() && + !HandledFirstNonOverlappingEmptyField && !IsOverlappingEmptyField; ---------------- 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. 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
