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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/array:245
 
+#ifndef _LIBCPP_CXX03_LANG
+    union __wrapper {
----------------
zoecarver wrote:
> zoecarver wrote:
> > zoecarver wrote:
> > > This should be after C++11. In C++11 this won't be an aggregate (`__w` is a non-static member without initialization). 
> > > __w is a non-static member without initialization.
> > 
> > This should say: "`__w` is a non-static member with default initialization."
> After a quick scan it looks like there is no test for `is_aggregate<array<T, N>>`. Any chance you could add that (note: I think it is important to have checks for both `is_aggregate<array<T, N>>` and `is_aggregate<array<T, 0>>`)? 
> This should be after C++11. In C++11 this won't be an aggregate (__w is a non-static member without initialization).

I don't think that's correct. I think it is an aggregate -- `__w` doesn't have a member initializer. Clang seems to agree with me using `__is_aggregate`. In all cases, I'm adding tests.


================
Comment at: libcxx/include/array:269
+    const value_type* data() const _NOEXCEPT {return reinterpret_cast<const value_type*>(__elems_);}
+#endif
+
----------------
zoecarver wrote:
> Nit: will you put a comment here?
IMO the code block is small enough that a comment on the closing `#endif` isn't useful.


================
Comment at: libcxx/test/std/containers/sequences/array/front_back_const.pass.cpp:24
+
+TEST_CONSTEXPR_CXX14 bool tests()
+{
----------------
miscco wrote:
> 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). 
> 
Yes it is -- initially `constexpr` implied `const` on variables, which sheds some light on why things evolved that way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80452