[libcxx-commits] [PATCH] D90999: [libc++] Implements ranges::enable_borrowed_range

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/span:525
+inline _LIBCPP_INLINE_VISIBILITY constexpr
+bool _VSTD::ranges::enable_borrowed_range<span<_Tp, _Extent>> = true;
+#endif // !defined(_LIBCPP_HAS_NO_RANGES)
----------------
Quuxplusone wrote:
> Nit: I'd prefer not to see `_VSTD::` here.
> Could someone (presumably @ldionne) comment on whether `_LIBCPP_INLINE_VISIBILITY` is a good idea here? My intuitive impression is "no, it shouldn't be here"; compare how we did it in `<numbers>`. (But those are partial specializations expressed via C++20 constraints, whereas this is a partial specialization expressed in the old-fashioned way. I don't know if that makes a subtle difference.)
> 
> (Note that `_LIBCPP_INLINE_VAR` is also a thing, but I think we don't want it here (because this is post-C++14-only) and my grepping didn't turn up any more precedents for what we're doing here with partial specialization.)
@ldionne also had doubts, https://reviews.llvm.org/D90999#inline-947749. If we don't do that in `<numbers>` then I'll remove them before committing. The same for the `_VSTD::`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90999