hubert.reinterpretcast added inline comments.

Comment at: clang/lib/AST/ASTContext.cpp:2416
-  // Double and long long should be naturally aligned if possible.
+  // Double, long double (only when the target supports AIX power alignment) 
+  // long long should be naturally aligned if possible.
I suggest to clarify the binding of the parenthetical and also to make the 
context that the required alignment is lower than the natural alignment more 
  // Double (and, for targets supporting AIX `power` alignment, long double) and
  // long long should be naturally aligned (despite requiring less alignment) if
  // possible.

Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1210
+  auto getBaseOrPreferredAlign = [&](CharUnits UnpackedAlign) {
+    return (Packed && ((Context.getLangOpts().getClangABICompat() <=
The lambda is currently being named for what is produced based on the identity 
of what gets passed to it and not named for what it does.

This should be named `getPackedBaseAlignFromUnpacked` or similar.

Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1212
+    return (Packed && ((Context.getLangOpts().getClangABICompat() <=
+                        LangOptions::ClangABI::Ver6) ||
+                       Context.getTargetInfo().getTriple().isPS4()))
I suggest not applying "ABI compat" with Clang <= 6 on AIX. There was is no 
Clang <= 6 with AIX support.

Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1218
   // Clang <= 6 incorrectly applied the 'packed' attribute to base classes.
   // Per GCC's documentation, it only applies to non-static data members.
This comment belongs inside the lambda.

Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1235
+  // Do not use AIX special alignment if current base is not the first member 
+  // the struct is not a union.
  // AIX `power` alignment does not apply the preferred alignment for non-union
  // classes if the source of the alignment (the current base in this context)
  // follows introduction of the first member with allocated space.

Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1769
+  bool FoundNonOverlappingEmptyField = false;
+  bool SupportsAIXPowerAlignment =
The name is wrong for the value (the value corresponding to this name is 
`!IsOverlappingEmptyField`). I would suggest something like 

Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1772
+      Context.getTargetInfo().defaultsToAIXPowerAlignment();
+  if (SupportsAIXPowerAlignment && !HandledFirstNonOverlappingEmptyField &&
+      !IsOverlappingEmptyField)
Move the definition of the variable here and just use the `if` condition as the 

  rG LLVM Github Monorepo


cfe-commits mailing list

Reply via email to