- Add UETT_PreferredAlignOf to account for the difference between __alignof and alignof
- AlignOfType now returns ABI alignment instead of preferred alignment iff clang-abi-compat > 7, and one uses _Alignof or alignof
Details
Diff Detail
- Repository
- rC Clang
Event Timeline
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.
include/clang/Basic/LangOptions.h | ||
---|---|---|
128 | Clang 7 was branched from SVN r338536. | |
131 | 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. | |
132 | 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). | |
284 | Please avoid reflowing unrelated lines; it makes future "svn annotate"s less useful. | |
include/clang/Basic/TypeTraits.h | ||
99–104 | Nit: comments should start with a capital letter and end with a period. Maybe also make these documentation comments (use ///)? | |
lib/AST/ExprConstant.cpp | ||
5960 | LLVM / Clang convention is to start variable names with a capital letter. |
Please extend the tests to check that the -fclang-abi-compat=7 switch undoes your change here.
One other comment, and otherwise this looks good. Will you need someone to commit this for you?
test/SemaCXX/align-x86.cpp | ||
---|---|---|
5–6 | It's not OK to include standard library headers here -- there's no guarantee that anything sensible will be on the include path, and we want our tests to be self-contained. (Feel free to define your own mini-std::complex in this test.) |
I'm having real difficulty with this - I get really odd errors in seemingly unrelated tests when I change things.
Is it possible you have LLVM and Clang checked out at different revisions? That's a common source of weird behavior, particularly in CodeGen tests.
Is it possible you have LLVM and Clang checked out at different revisions? That's a common source of weird behavior, particularly in CodeGen tests.
Unfortunately no - when I don't specifically don't set the type trait kind of __alignof to UETT_PreferredAlignOf, and remove the code in AlignOfType to differentiate between PreferredAlignOf and AlignOf, all the tests pass again.
You've extended UnaryExprOrTypeTrait from having 4 enumerators to 5, which means it no longer fits in a 2-bit bitfield. Stmt::UnaryExprOrTypeTraitExprBitfields::Kind needs to be made wider.
Finish doing the thing! all tests are passing, at least on the revision I'm running on :)
@rsmith please check the thing!
I don't actually know how to send this upstream, since it's been accepted... How should I proceed?
If you don't have an SVN account, you can ask someone who does to commit it for you. (In this case, I'll do it.)
A few minor nit first, though :)
docs/ReleaseNotes.rst | ||
---|---|---|
80 | I don't think we need to say this. It could be set to 7 before, it just had no effect. | |
151 | which -> that | |
include/clang/Basic/DiagnosticSemaKinds.td | ||
5478 | I think you may have missed this comment. |
I'm surprised that nobody discussed the potential for ABI breakage of this change, which changes the result of the alignof operator. I'm trying to evaluate what to do in libc++ as a result of this change (now std::alignment_of does not match alignof) for PR39713, and I'm wondering whether this issue was considered here. I'm not saying this is a bad change, by the way, I'd simply like to understand what the implications are and find good arguments to convince me to follow the lead in libc++. Pinging @dexonsmith for awareness.
The ABI breakage was already there, in the difference between GCC and Clang
- if one compiles against libc++ with gcc, it is not compatible with things
that are compiled with clang. I frankly think that being ABI compatible
with gcc (and correct according to the standard) is far more important than
being ABI compatible with previous versions of clang - if people need the
ABI compatibility, they can and should use -fclang-abi-compat
I don't think we need to say this. It could be set to 7 before, it just had no effect.