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.



cfe-commits mailing list

Reply via email to