mclow.lists added a comment.
Herald added subscribers: bixia, jlebar.
In general, this looks good to me.
I suggested a lot of test improvements, but not very much to the code.
Needs the same kind of namespace changes in the tests as in
https://reviews.llvm.org/D41376, but that's a minor thing.
================
Comment at: libcxx/include/experimental/simd:1068
static typename std::enable_if<simd<_Up, _Abi>::size() ==
simd<_Tp, _NewAbi>::size(),
simd<_Tp, _NewAbi>>::type
----------------
Weird indentation in the `enable_if`
================
Comment at: libcxx/test/std/experimental/simd/simd.casts/simd_cast.pass.cpp:19
#include <cstdint>
using namespace std::experimental::parallelism_v2;
----------------
All the tests should `#include "test_macros.h"`
================
Comment at: libcxx/test/std/experimental/simd/simd.casts/simd_cast.pass.cpp:22
+template <class T, class... Args>
+auto unsupported_cast(Args&&... args)
----------------
I think this is worth a comment here - "This conversion is deleted so that it
prevents ..."
================
Comment at:
libcxx/test/std/experimental/simd/simd.casts/static_simd_cast.pass.cpp:33
static_assert(
std::is_same<decltype(static_simd_cast<simd<float, simd_abi::scalar>>(
fixed_size_simd<int, 1>())),
----------------
General comment: We have a macro called `ASSERT_SAME_TYPE` that makes this
clearer.
Instead of all this, you can just write:
ASSERT_SAME_TYPE(decltype(static_simd_cast<simd<float,
simd_abi::scalar>>(fixed_size_simd<int, 1>())), simd<float, simd_abi::scalar);
================
Comment at:
libcxx/test/std/experimental/simd/simd.casts/to_compatible.pass.cpp:39
+
+void test_to_compatible_extension() {
+ auto arr = split_by<32 / simd<int>::size()>(
----------------
Since this is an extension, then the test needs to live in test/libcxx, not
test/std.
(and should probably be two tests, one in test/std, and one in test/libcxx).
Same for to_native.pass.cpp below.
================
Comment at:
libcxx/test/std/experimental/simd/simd.casts/to_fixed_size.pass.cpp:30
+void test_to_fixed_size() {
+ auto v = to_fixed_size(native_simd<int>([](int i) { return i; }));
+ static_assert(std::is_same<fixed_size_simd<int, native_simd<int>::size()>,
----------------
All of these tests need to check the "noexcept-ness" of the operation.
Fortunately, we have a macro for that, too:
ASSERT_NOEXCEPT ( to_fixed_size(std::declval<native_simd<int>>()));
ASSERT_SAME_TYPE(decltype(to_fixed_size(std::declval<native_simd<int>>())),
fixed_size_simd<int, native_simd<int>::size()>);
https://reviews.llvm.org/D41415
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
