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

[Michael Schellenberger Costa via Phabricator via libcxx-commits]

miscco added inline comments.


================
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
----------------
ldionne wrote:
> 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.
I would like to note that this will break users code once they want to try different standard library implementations.

Given that the other two major implementations use `nullptr` as return value this is a clear portability issue. Is there any other benefit to doing so?

I would note that 
a. This seems like an supremely rare case anyway
b. I would consider breaking code that relies on UB to be less or equally severe than breaking code when switching standard libraries 

That said I wouldn't shed any tears if you go with that implementation


================
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()
----------------
zoecarver wrote:
> ldionne wrote:
> > 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.
> I don't think the template is needed.
But if one chooses the "right" TEST_CONSTEXPR_CXX macro this should not happen?  


================
Comment at: libcxx/test/std/containers/sequences/array/array.data/data.pass.cpp:37
+template <typename Dummy = void>
+TEST_CONSTEXPR_CXX14 bool tests()
 {
----------------
ldionne wrote:
> 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).
Again, Why not simply declare this as TEST_CONSTEXPR_CXX17 and be correct in every case. Is there any benefit to not doing so?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80452