[libcxx-commits] [PATCH] D80452: [libc++] Complete overhaul of constexpr support in std::array

[Michael Schellenberger Costa via Phabricator via libcxx-commits]

miscco added a comment.

I had two minor nits and a general OMG moment about the diverging constexpr status wrt indexing .

Besides that LGTM



================
Comment at: libcxx/test/std/containers/sequences/array/array.data/data_const.pass.cpp:61
     }
 #if TEST_STD_VER > 14
     {
----------------
Nit: In the other C++17 defines you use `#if TEST_STD_VER >= 17` and not `#if TEST_STD_VER > 14`


================
Comment at: libcxx/test/std/containers/sequences/array/array.data/data_const.pass.cpp:67
 
-        static_assert (  c1.data()  == &c1[0], "");
-        static_assert ( *c1.data()  ==  c1[0], "");
-        static_assert (  c2.data()  == &c2[0], "");
-        static_assert ( *c2.data()  ==  c2[0], "");
+        static_assert( c1.data() == &c1[0], "");
+        static_assert(*c1.data() ==  c1[0], "");
----------------
Nit: This always tests compile time. It would be more consistent with the other tests to use `assert` here and test the `constexpr` behavior via the `static_assert(tests(), "");`


================
Comment at: libcxx/test/std/containers/sequences/array/front_back_const.pass.cpp:24
+
+TEST_CONSTEXPR_CXX14 bool tests()
+{
----------------
Comment: I wrote multiple comments to check whether this is really different. than the non-const version. 

Checking with the standard shows it indeed is (terrible). 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80452/new/

https://reviews.llvm.org/D80452