[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1884
+      if (const BuiltinType *BTy =
+              Context.getBaseElementType(CTy->getElementType())
+                  ->getAs<BuiltinType>())
----------------
I don't think there's a reason to use `getBaseElementType` (which is used to handle arrays) on the element type of a `ComplexType`.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1885
+              Context.getBaseElementType(CTy->getElementType())
+                  ->getAs<BuiltinType>())
+        if (BTy->getKind() == BuiltinType::Double ||
----------------
I believe `castAs` should be expected to succeed here.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1888
+            BTy->getKind() == BuiltinType::LongDouble) {
+          PreferredAlign = CharUnits::fromQuantity(8);
+        }
----------------
I believe an assertion that `PreferredAlign` was 4 would be appropriate.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1893
+                       ->getAs<BuiltinType>()) {
+      if (BTy->getKind() == BuiltinType::Double ||
+          BTy->getKind() == BuiltinType::LongDouble) {
----------------
Use a lambda instead of duplicating the code.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1898
+    } else if (const RecordType *RT = D->getType()
+                                          ->getBaseElementTypeUnsafe()
+                                          ->getAs<RecordType>()) {
----------------
Is there a reason to use `getBaseElementTypeUnsafe` for this case and `Context.getBaseElementType` for the other ones? Also, it would make sense to factor out the array-type considerations once at the top of the if-else chain instead of doing so in each alternative.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1900
+                                          ->getAs<RecordType>()) {
+      if (const RecordDecl *RD = RT->getDecl()) {
+        const ASTRecordLayout &FieldRecord = Context.getASTRecordLayout(RD);
----------------
I'd be a bit concerned if this failed. Can we assert that we get a non-null pointer back?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79719/new/

https://reviews.llvm.org/D79719