hubert.reinterpretcast added inline comments.
================ Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1246 + + // AIX `power` alignment does not apply the preferred alignment for + // non-union classes if the source of the alignment (the current base in ---------------- Move the comment to above the previous `if` and make the following `if` the `else` of the previous `if`. ================ Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1796 + bool FoundFirstNonOverlappingEmptyFieldToHandle = + DefaultsToAIXPowerAlignment && FieldOffset == CharUnits::Zero() && + !HandledFirstNonOverlappingEmptyField && !IsOverlappingEmptyField; ---------------- Xiangling_L wrote: > 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. I'm not seeing the change for https://reviews.llvm.org/D79719?id=276143#inline-767942? ================ Comment at: clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp:19 + char x alignas(4)[8]; +} ; + ---------------- Minor nit: Remove the space before the semicolon. ================ Comment at: clang/test/Layout/aix-Wpacked-no-diagnostics.cpp:15 + +int a = sizeof(QQ); ---------------- Is there a reason to drop the `FileCheck` checking for the layout? 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
