[PATCH] D53207: Fix bug 26547 - alignof should return ABI alignment, not preferred alignment

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks, this generally looks good, but it needs tests. For `_Alignof`, test/Sema/align-x86.c` would be a reasonable place for the test; for C++ `alignof`, I don't see a suitable existing test file, but you could add one to test/SemaCXX, perhaps based on test/Sema/align-x86.c.



================
Comment at: include/clang/Basic/LangOptions.h:128
+    /// Attempt to be ABI-compatible with code generated by Clang 7.0.x
+    /// (SVN r344257). This causes alignof (C++) and _Alignof (C11) to be
+    /// compatible with __alignof (i.e., return the preferred alignment)
----------------
Clang 7 was branched from SVN r338536.


================
Comment at: include/clang/Basic/LangOptions.h:131
+    /// rather than returning the required alignment.
+    /// see https://bugs.llvm.org/show_bug.cgi?id=26547 for explanation
+    Ver7,
----------------
Nit: Capitalize "See", add period. But I think you can actually just drop this line: we don't really need to justify why we now follow the rules of the relevant language standards.


================
Comment at: include/clang/Basic/LangOptions.h:132
+    /// see https://bugs.llvm.org/show_bug.cgi?id=26547 for explanation
+    Ver7,
+
----------------
You will also need to update the code in lib/Frontend/CompilerInvocation.cpp to use this new enumerator (search for `Ver6` to find the code you need to change).


================
Comment at: include/clang/Basic/LangOptions.h:284
+  FPOptions()
+      : fp_contract(LangOptions::FPC_Off), fenv_access(LangOptions::FEA_Off) {}
 
----------------
Please avoid reflowing unrelated lines; it makes future "svn annotate"s less useful.


================
Comment at: include/clang/Basic/TypeTraits.h:99-104
+    // used for GCC's __alignof,
+    UETT_PreferredAlignOf,
+    // used for C's _Alignof and C++'s alignof
+    // this distinction is important because __alignof
+    // returns preferred alignment
+    // _Alignof and alignof return the required alignment
----------------
Nit: comments should start with a capital letter and end with a period. Maybe also make these documentation comments (use `///`)?


================
Comment at: lib/AST/ExprConstant.cpp:5960
+
+  const bool alignOfReturnsPreferred =
+      Info.Ctx.getLangOpts().getClangABICompat() <= LangOptions::ClangABI::Ver7;
----------------
LLVM / Clang convention is to start variable names with a capital letter.


Repository:
  rC Clang

https://reviews.llvm.org/D53207