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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
