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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In D105597#2864592 <https://reviews.llvm.org/D105597#2864592>, @ldionne wrote:

> 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.

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.



================
Comment at: libcxx/include/variant:242
+struct __light_array {
+  _Tp __buff_[_Size] = {0};
+
----------------
Quuxplusone wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > 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?
> > 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.
> @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.)
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.


================
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;
----------------
ldionne wrote:
> 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.
Thanks for investigating @ldionne. I agree a `static_assert` makes the intention clear.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105597