[libcxx][nfc] Remove <variant>'s dependence on <array>.

[zoecarver]

Summary

This will allow us to use variant in common_iterator. We do this by introducing a new __light_array type that variant uses instead of std::array.

[cjdb]

LGTM if, and only if, we explicitly document that <array> is removed from <variant> in our release notes.

[• Quuxplusone]

libcxx/include/variant

242

What is _Tp here? Are we sure 0 is convertible to _Tp? (Is _Tp always just size_t? That'd be super convenient. :))
Are we sure _Size is never zero?

244

Nit: here and line 249, inline is not needed because these methods are implicitly inline.

254–261

I wonder why we need == to work on this type.

541

I see no reason to drop the double-{{ here. I think it works either way, but the single-{ version is using magic brace-inferring logic.

[Mordante]

In general LGTM. I'm only concerned about the case where`_Size == 0`.

libcxx/include/variant

242

Since the variable is public please drop the trailing underscore. Small nit, I would prefer __buf instead of __buff, the latter looks weird to me.

[• Quuxplusone]

libcxx/include/variant

242

@Mordante: I'd strongly prefer to keep the trailing underscore on data members, to distinguish e.g. the data member __buf_ from the parameter __buf. (And I don't think we can make it private because then this type wouldn't get aggregate initialization.)

254–261

It turns out we do not need == to work on this type. Please remove.
Also remove operator[] (non-const version).
It turns out that _Tp is always either a function-pointer type, or another __light_array.
The result:

template<class _Tp, size_t _Size>
struct __light_array {
  static_assert(_Size > 0);
  _Tp __buf_[_Size] = {};

  _LIBCPP_INLINE_VISIBILITY constexpr
  const _Tp& operator[](size_t __n) const noexcept {
      return __buf_[__n];
  }
};

[ldionne]

In D105597#2863350, @cjdb wrote:

LGTM if, and only if, we explicitly document that <array> is removed from <variant> in our release notes.

There's little cost in documenting that in our release notes so we might as well do it, however I want to point out that we have started going crazy over something that we previously paid minimal attention to. I get that we want to do the right thing, and that large changes such as @cjdb's massive IWYU in D104980 make us think about this issue more seriously, but we should not become obsessed with that either.

So let's add a release note and call it a day. If we do want to do something more systematic like D104980, then yeah, I agree we should think about it more deeply because the breakage could be large enough to seriously hinder adoption of libc++-next by downstream users, but for a small change like this, let's not overthink it.

libcxx/include/variant

254–261

Agreed, let's try to make this as small and simple as possible.

Note to self and reviewers: I just checked, and I'm pretty confident Size > 0 is always true. There's always at least one element in the N-dimensional array -- the degenerate case is std::visit(f) (without any variant), but there's still one element that dispatches to f() in the array. Let's add a static_assert just for clarity.

[zoecarver]

Apply review feedback.

[zoecarver]

• Update the release notes.

[Mordante]

In D105597#2864592, @ldionne wrote:

In D105597#2863350, @cjdb wrote:

LGTM if, and only if, we explicitly document that <array> is removed from <variant> in our release notes.

There's little cost in documenting that in our release notes so we might as well do it, however I want to point out that we have started going crazy over something that we previously paid minimal attention to. I get that we want to do the right thing, and that large changes such as @cjdb's massive IWYU in D104980 make us think about this issue more seriously, but we should not become obsessed with that either.

So let's add a release note and call it a day. If we do want to do something more systematic like D104980, then yeah, I agree we should think about it more deeply because the breakage could be large enough to seriously hinder adoption of libc++-next by downstream users, but for a small change like this, let's not overthink it.

I agree. If the user's code didn't include the required headers the code wasn't portable in the first place. There's no guarantee other Standard library vendors include the same set of not-required headers in their implementation. It would be great when we can issue proper diagnostics for these large changes.

In the past I've had this issue after upgrading my GCC toolchain; libstdc++ internally removed a lot of headers and parts of my code no longer compiled. Fixing it was trivial.

libcxx/include/variant

242

I had a look and it seems std::array also uses a trailing underscore for its public member, so in that case I no longer have any objections against the trailing underscore. Still prefer __buf_ over __buff_.

I agree the member should be public, it was not my intention to imply it should become private.

254–261

Thanks for investigating @ldionne. I agree a static_assert makes the intention clear.

[Mordante]

LGTM!

libcxx/include/variant

242

@zoecarver It seem I typed my previous message while you posted a new version. So that issue has been addressed.

[zoecarver]

Update static_assert message.

Rename to __farray.

Update comment above farray.

[• Quuxplusone]

libcxx/include/variant

526

It occurs to me that you could eliminate __farray::operator[], and just use __elems.__buf_[__index] right here, and on line 490 use __fdiagonal.__buf_[__index] instead of __fdiagonal[__index]. Then __farray would be a two-liner, and I think that'd be nice.
This isn't a blocker, but I'd like to see it done in a followup commit unless @ldionne thinks it would not be a good idea.

libcxx/include/variant

 } // namespace std
 
 */
 
 #include <__availability>
 #include <__config>
 #include <__utility/forward.h>
 #include <__tuple>
-#include <array>
 #include <compare>
 #include <exception>
 #include <functional>
 #include <initializer_list>
 #include <limits>
 #include <new>
 #include <tuple>
 #include <type_traits>

 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 // TODO: GCC 5 lies about its support for C++17 (it says it supports it but it
 //       really doesn't). That breaks variant, which uses some C++17 features.
 //       Remove this once we drop support for GCC 5.
 #if _LIBCPP_STD_VER > 14 && !(defined(_LIBCPP_COMPILER_GCC) && _GNUC_VER_NEW < 6000)
 
+template<class _Tp, size_t _Size>
+struct __light_array {
+  _Tp __buff_[_Size] = {0};
+
+  inline _LIBCPP_INLINE_VISIBILITY constexpr
+  _Tp &operator[](size_t __n) noexcept {
+      return __buff_[__n];
+  }
+
+  inline _LIBCPP_INLINE_VISIBILITY constexpr
+  const _Tp &operator[](size_t __n) const noexcept {
+      return __buff_[__n];
+  }
+
+  inline _LIBCPP_INLINE_VISIBILITY
+  constexpr friend bool
+  operator==(const __light_array& __x, const __light_array& __y) noexcept {
+      for (size_t __i = 0; __i < _Size; ++__i)
+        if (__x.__buff_[__i] != __y.__buff_[__i])
+          return false;
+      return true;
+  }
+};
+
 _LIBCPP_NORETURN
 inline _LIBCPP_INLINE_VISIBILITY
 _LIBCPP_AVAILABILITY_THROW_BAD_VARIANT_ACCESS
 void __throw_bad_variant_access() {
 #ifndef _LIBCPP_NO_EXCEPTIONS
         throw bad_variant_access();
 #else
         _VSTD::abort();

 
 private:
   template <class _Tp>
   inline _LIBCPP_INLINE_VISIBILITY
   static constexpr const _Tp& __at(const _Tp& __elem) { return __elem; }
 
   template <class _Tp, size_t _Np, typename... _Indices>
   inline _LIBCPP_INLINE_VISIBILITY
-  static constexpr auto&& __at(const array<_Tp, _Np>& __elems,
+  static constexpr auto&& __at(const __light_array<_Tp, _Np>& __elems,
                                size_t __index, _Indices... __indices) {
     return __at(__elems[__index], __indices...);
   }
 
   template <class _Fp, class... _Fs>
   static constexpr void __std_visit_visitor_return_type_check() {
     static_assert(
         __all<is_same_v<_Fp, _Fs>...>::value,
         "`std::visit` requires the visitor to have a single return type.");
   }
 
   template <class... _Fs>
   inline _LIBCPP_INLINE_VISIBILITY
   static constexpr auto __make_farray(_Fs&&... __fs) {
     __std_visit_visitor_return_type_check<__uncvref_t<_Fs>...>();
-    using __result = array<common_type_t<__uncvref_t<_Fs>...>, sizeof...(_Fs)>;
-    return __result{{_VSTD::forward<_Fs>(__fs)...}};
+    using __result = __light_array<common_type_t<__uncvref_t<_Fs>...>, sizeof...(_Fs)>;
+    return __result{_VSTD::forward<_Fs>(__fs)...};
   }
 
   template <size_t... _Is>
   struct __dispatcher {
     template <class _Fp, class... _Vs>
     inline _LIBCPP_INLINE_VISIBILITY
     static constexpr decltype(auto) __dispatch(_Fp __f, _Vs... __vs) {
         return _VSTD::__invoke_constexpr(