This patch uses C++11 parameter packs and constexpr functions to allow AlignedCharArrayUnion to hold an arbitrary number of types.
Details
Diff Detail
Event Timeline
Hmm - did have some comments, but I think, maybe, all the supported compilers support std::aligned_union now, so switching to that might be an/the right option now? *hopeful*
include/llvm/Support/AlignOf.h | ||
---|---|---|
131 | Leave a non-doxy comment about how std::aligned_union could be used once LLVM's supported compilers support it. (GCC 4.8 is what we'd need, I think - oh, 4.8 is our minimum) | |
unittests/ADT/AlignOfTest.cpp | ||
31–34 | These shouldn't need to be in a class like this, should they? | |
36–41 | Not sure this test is needed/covers anything - tests that memset works, really? But, yeah... not sure what the right/appropriate testing would be for this. | |
38 | Should that be sizeof(MyU.buffer) rather than sizeof(U)? (it could have extra padding, etc - I guess not with this specific set of types) |
I agree with David here. Can you check if std::aligned_union actually works with 4.8 and the minimum version of MSVC we support (I think 2015) and switch to it instead?
According to the GCC 5 changelist, support for std::aligned_union was introduced with GCC 5.
I was able to verify with Compiler Explorer that std::aligned_union is unsupported (as in, not found) by 4.9.5. It is supported by 5.1.
According to Microsoft, Visual Studio 2015 supports std::aligned_union.
I was able to verify with "Compile Visual Studio C++ Online" that Visual C++ 2015 supports it.
Oh, too bad.
According to Microsoft, Visual Studio 2015 supports std::aligned_union.
I was able to verify with "Compile Visual Studio C++ Online" that Visual C++ 2015 supports it.
Thanks for checking.
I'm fine with this going in as long as David is happy.
Where do you actually plan to use this? In LLVM proper or in some downstream project?
Please include a comment in the header/near/in the definition of AlignedCharArrayUnion about the minimum compiler version before we can switch to std::aligned_union.
My plan was to use this to implement std::variant (a C++17 feature) on the cheap inside LLVM. I mentioned wanting to implement a tagged union and Lang Hames said it would be useful in several places. Hence the review.
Added a comment at David's request, and moved the test into the Support/AlignOf unit tests. Also removed the memcpy test.
This failed to build under Visual Studio:
C:\PROGRA~2\MICROS~1.0\VC\bin\AMD64_~2\cl.exe /nologo /TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib\Support -IC:\b\slave\sanitizer-windows\llvm\lib\Support -Iinclude -IC:\b\slave\sanitizer-windows\llvm\include /DWIN32 /D_WINDOWS /W4 -wd4141 -wd4146 -wd4180 -wd4244 -wd4258 -wd4267 -wd4291 -wd4345 -wd4351 -wd4355 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4800 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4324 -w14062 -we4238 /Zc:inline /Zc:strictStrings /Oi /Zc:rvalueCast /MD /O2 /Ob2 -UNDEBUG /EHs-c- /GR- /showIncludes /Folib\Support\CMakeFiles\LLVMSupport.dir\CommandLine.cpp.obj /Fdlib\Support\CMakeFiles\LLVMSupport.dir\ /FS -c C:\b\slave\sanitizer-windows\llvm\lib\Support\CommandLine.cpp C:\b\slave\sanitizer-windows\llvm\include\llvm/Support/AlignOf.h(135): error C2027: use of undefined type 'llvm::detail::AlignerImpl<Ts...>' C:\b\slave\sanitizer-windows\llvm\include\llvm/Support/AlignOf.h(135): note: see declaration of 'llvm::detail::AlignerImpl<Ts...>' C:\b\slave\sanitizer-windows\llvm\include\llvm/Support/AlignOf.h(136): note: see reference to class template instantiation 'llvm::AlignedCharArrayUnion<Ts...>' being compiled ninja: build stopped: subcommand failed.
I did some restricted testing with http://rextester.com/l/cpp_online_compiler_visual but it looks like that wasn't sufficient.
Is it compatible to -m32? See also; http://bb.pgr.jp/builders/ninja-x64-msvc-RA-centos6/builds/33628
This seems to be the source of new test failure on 32-bit x86:
[==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from AlignOfTest [ RUN ] AlignOfTest.BasicAlignedArray /var/tmp/portage/sys-devel/llvm-9999/work/llvm-9999/unittests/Support/AlignOfTest.cpp:151: Failure Expected: alignof(T<long long>) Which is: 4 To be equal to: alignof(AlignedCharArrayUnion<long long>) Which is: 8 /var/tmp/portage/sys-devel/llvm-9999/work/llvm-9999/unittests/Support/AlignOfTest.cpp:153: Failure Expected: alignof(T<double>) Which is: 4 To be equal to: alignof(AlignedCharArrayUnion<double>) Which is: 8 [ FAILED ] AlignOfTest.BasicAlignedArray (1 ms)
It should be noted that this specific commit fails due to assertion:
/home/mgorny/llvm/unittests/Support/AlignOfTest.cpp:110:1: error: static assertion failed: Statically-computed alignment must be right static_assert(alignof(U) == alignof(uint64_t), "Statically-computed alignment must be right"); ^
However, rL291515 (@rnk) following it builds fine and has this test failure.
Leave a non-doxy comment about how std::aligned_union could be used once LLVM's supported compilers support it. (GCC 4.8 is what we'd need, I think - oh, 4.8 is our minimum)