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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


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


================
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:
> 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];
  }
};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105597