[libcxx-commits] [PATCH] D105597: [libcxx][nfc] Remove <variant>'s dependence on <array>.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

In D105597#2863350 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.



================
Comment at: libcxx/include/variant:254-261
+  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;
----------------
Quuxplusone wrote:
> Quuxplusone wrote:
> > I wonder why we need `==` to work on this type.
> 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];
>   }
> };
> ```
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105597