timshen added inline comments.

================
Comment at: libcxx/include/experimental/simd:726
+#if defined(_LIBCPP_COMPILER_CLANG)
+#define _SPECIALIZE_VEC_EXT(_TYPE, _NUM_ELEMENT)                               
\
+  template <>                                                                  
\
----------------
mclow.lists wrote:
> When we define user-visible macros in libc++, we prepend them with _LIBCPP_ 
> to avoid collisions.
> Accordingly, `_SPECIALIZE_VEC_EXT` should be named 
> `_LIBCPP_SPECIALIZE_VEC_EXT`
> 
Well _SPECIALIZE_VEC_EXT and _SPECIALIZE_VEC_EXT_32 are not user-visible, as 
they are (1) '#undef'ed after all uses, and (2) they start with underscore, so 
don't collide with existing user-defined macros.


================
Comment at: libcxx/include/experimental/simd:1341
 // [simd.class]
 // TODO: implement simd
 template <class _Tp, class _Abi>
----------------
mclow.lists wrote:
> Is this TODO still necessary?
I think so, as some operations are still not implemented, for example 
operator++().


================
Comment at: libcxx/test/std/experimental/simd/simd.cons/default.pass.cpp:21
+using namespace std::experimental::parallelism_v2;
+
+int main() { (void)native_simd<int32_t>(); }
----------------
mclow.lists wrote:
> Do we need any other ctors tested here? `fixed_size_simd<int32_t, 4>` for 
> example?
> Are there any post-conditions on the object created?
> 
> calling `size()` for example?
> 
Yes. Added more.

The test space is large and it's hard to test everything. I went "every line of 
the implementation should be covered" standard with myself, and only had 
native_simd's default ctor. fixed_size_simd<>'s ctor is exactly implemented by 
the same line, so I didn't bother.

But now the attention is already raised, it doesn't hurt to add more tests.


================
Comment at: libcxx/test/std/experimental/simd/simd.cons/default.pass.cpp:21
+using namespace std::experimental::parallelism_v2;
+
+int main() { (void)native_simd<int32_t>(); }
----------------
timshen wrote:
> mclow.lists wrote:
> > Do we need any other ctors tested here? `fixed_size_simd<int32_t, 4>` for 
> > example?
> > Are there any post-conditions on the object created?
> > 
> > calling `size()` for example?
> > 
> Yes. Added more.
> 
> The test space is large and it's hard to test everything. I went "every line 
> of the implementation should be covered" standard with myself, and only had 
> native_simd's default ctor. fixed_size_simd<>'s ctor is exactly implemented 
> by the same line, so I didn't bother.
> 
> But now the attention is already raised, it doesn't hurt to add more tests.
Yes, size() is the post-condition. There is not much beyond that - the elements 
are not even value initialized.


https://reviews.llvm.org/D41376



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to