Xiangling_L added inline comments.
================ Comment at: clang/lib/AST/ASTContext.cpp:2409 + const RecordDecl *RD = RT->getDecl(); + return std::max(ABIAlign, static_cast<unsigned>(toBits( + getASTRecordLayout(RD).PreferredAlignment))); ---------------- hubert.reinterpretcast wrote: > hubert.reinterpretcast wrote: > > Please add a comment regarding the situations where the `ABIAlign` value is > > greater than the `PreferredAlignment` value. It may be appropriate to > > assert that, absent those cases, the `PreferredAlignment` value is at least > > that of `ABIAlign`. > It does not appear that the maximum of the two values is the correct answer: > ``` > struct C { > double x; > } c; > typedef struct C __attribute__((__aligned__(2))) CC; > > CC cc; > extern char x[__alignof__(cc)]; > extern char x[2]; // this is okay with IBM XL C/C++ > ``` > Please add a comment regarding the situations where the ABIAlign value is > greater than the PreferredAlignment value. I added a `if` condition to guard the situation where `ABIAlign` should be returned instead of adding a comment. Please let me know if that is sufficient. ================ Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1888 + BTy->getKind() == BuiltinType::LongDouble) { + PreferredAlign = CharUnits::fromQuantity(8); + } ---------------- hubert.reinterpretcast wrote: > hubert.reinterpretcast wrote: > > I believe an assertion that `PreferredAlign` was 4 would be appropriate. > It seems that overriding the value should only be done after additional > checks: > ``` > typedef double __attribute__((__aligned__(2))) Dbl; > struct A { > Dbl x; > } a; > extern char x[__alignof__(a)]; > extern char x[2]; // this is okay with IBM XL C/C++ > ``` > > I am getting concerned that the logic here overlaps quite a bit with > `getPreferredTypeAlign` and refactoring to make the code here more common > with `getPreferredTypeAlign` is necessary. Fixed the typedef related cases with my new changes, and the overlaps were not a lot as I expected. So I didn't do any refactoring yet. Please let me know if you still think it's necessary to refactor the code somehow. 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