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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

Thanks @zoecarver and @miscco for the comments and review! I'm addressing your comments.

In D80452#2051973 <https://reviews.llvm.org/D80452#2051973>, @miscco wrote:

> What I completely forgot, thisimplements the array portion of the [[ http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1032r1.html | misc_constexpr  ]]paper so you should adopt/add the feature test macro ````__cpp_lib_array_constexpr = 201811L```


Good catch! Will add in the next update.



================
Comment at: libcxx/include/array:246
+    struct _Empty { };
+    typedef typename conditional<is_const<_Tp>::value, const _Empty,
+                                _Empty>::type _CharType;
----------------
zoecarver wrote:
> zoecarver wrote:
> > Why use a new `_Empty` struct? `char` makes more sense and has a defined size in the standard. 
> Nit: this could be `add_const`.
Default constructing a `char` leaves it to an undefined value, which you can't do inside `constexpr`. So something like `std::array<int, 0> a;` isn't valid inside `constexpr` if the empty-array implementation holds a `char` internally, but it does work with an empty struct.

But regardless, this is being undone anyway since I'm moving to the `union` trick.


================
Comment at: libcxx/include/array:352
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
+    value_type* data() _NOEXCEPT {return nullptr;}
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
----------------
zoecarver wrote:
> This will probably break some people's code. I would much prefer not returning `nullptr` here. What's the downside of using something similar to D60666's implementation of `data()`?
Indeed -- I spent some more time on this and it does work. Updated.


================
Comment at: libcxx/test/std/containers/sequences/array/array.cons/deduct.pass.cpp:33
 
-int main(int, char**)
+template <typename Dummy = void>
+constexpr bool tests()
----------------
miscco wrote:
> being new to the party, what is the reason for the template?
It's to avoid this error:

```
at.pass.cpp:27:27: error: constexpr function never produces a constant expression [-Winvalid-constexpr]
TEST_CONSTEXPR_CXX14 bool tests()
```

It triggers when e.g. `at()` is not marked as `constexpr` in the given standard mode, but the `tests()` function that calls it is.

However, in this specific case, it's not useful because the test is only enabled in c++17 and above, so there's no `constexpr` mismatch. But it is useful in other tests, like the ones for `at`, `data` and probably others. I'll remove the unnecessary ones.


================
Comment at: libcxx/test/std/containers/sequences/array/array.cons/implicit_copy.pass.cpp:39
+    {
+        typedef double T;
+        typedef std::array<T, 3> C;
----------------
miscco wrote:
> Would it make sense to consistently drop all those typedefs?
> 
> If not what is their purpose?
I think it would make sense to drop them -- I'm not a big fan. It's just the way the tests were written originally I guess. I don't really want to systematically remove the typedefs in this patch, though.


================
Comment at: libcxx/test/std/containers/sequences/array/array.cons/initializer_list.pass.cpp:27
         typedef std::array<T, 3> C;
-        C c = {1, 2, 3.5};
+        C const c = {1, 2, 3.5};
         assert(c.size() == 3);
----------------
miscco wrote:
> preexisting: Are those conversions from into double intentional?
I am pretty sure they are, we're likely testing that aggregate initialization works as expected.


================
Comment at: libcxx/test/std/containers/sequences/array/array.data/data.pass.cpp:37
+template <typename Dummy = void>
+TEST_CONSTEXPR_CXX14 bool tests()
 {
----------------
miscco wrote:
> This is `TEST_CONSTEXPR_CXX14` but the constexpr test is `#if TEST_STD_VER >= 17`
> 
> Could we define a macro `TEST_CONSTEXPR_CXX14`?
> 
> Notably this should not compile for C++14 as data is not constexpr.
> 
> Occurs at other places too
> Could we define a macro `TEST_CONSTEXPR_CXX14`?

Do you mean `TEST_CONSTEXPR_CXX17`?

I'm making the test code `constexpr` whenever it can be marked as such, I'm just not checking the constexpr-ness of `std::array` itself unless the Standard mandates it. That's the purpose of the `Dummy` template parameter -- it makes it OK to have `constexpr` on the `tests()` function in C++14, despite its body not being `constexpr`-friendly (because `data()` isn't `constexpr` as you mention).


================
Comment at: libcxx/test/std/containers/sequences/array/array.data/data.pass.cpp:53
         T* p = c.data();
-        LIBCPP_ASSERT(p != nullptr);
+        (void)p;
     }
----------------
zoecarver wrote:
> I don't see the point of this test anymore (lines 48-54). 
I've re-added the check for `p != nullptr`, so this is moot.


================
Comment at: libcxx/test/std/containers/sequences/array/array.data/data_const.pass.cpp:95
+        const T* p = c.data();
+        std::uintptr_t pint = reinterpret_cast<std::uintptr_t>(p);
+        assert(pint % TEST_ALIGNOF(T) == 0);
----------------
zoecarver wrote:
> `pint` will always be zero, no? I think it would make more sense to test this with non-null data (`std::array<T, 1>` maybe). 
Moot since I'm using the `union` trick.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80452