[libc++][NFC] Add namespace comments in ranges

[philnik]

Summary

With this patch there should be no more namespaces without closing comment

[philnik]

libcxx/include/__ranges/reverse_view.h

117

Here, in line 123 and line 129 clang-tidy claims these definitions can lead to ODR-violations. Is inline implied by it being a template or something else and therefore this is a clang-tidy bug or is this actually potentially an ODR-violation and this is a libc++ bug?

[ldionne]

The patch itself LGTM, but there's indeed an interesting side question about the linkage of variable templates.

libcxx/include/__ranges/reverse_view.h

117

I'll use cppreference instead of the standard here because everything I found in the Standard was much harder to understand than cppreference.

My understanding is that the __is_reverse_view base template has the usual linkage for templates, which is extern inline (more precisely linkonce_odr in LLVM lingo). See external linkage at https://en.cppreference.com/w/cpp/language/storage_duration.

However, the __is_reverse_view<reverse_view<_Tp>> specialization is a variable, not a variable template, so this part of https://en.cppreference.com/w/cpp/language/storage_duration / internal linkage applies to it instead:

non-volatile non-template non-inline non-exported const-qualified variables (including constexpr) that aren't declared extern and aren't previously declared to have external linkage;

Naively, this would mean that __is_reverse_view<reverse_view<_Tp>> has internal linkage, and hence clang-tidy is right to flag this.

However, according to https://stackoverflow.com/a/48078494/627587, it appears that the rule doesn't actually apply, and so clang-tidy would in fact be wrong to flag this. Perhaps clang-tidy doesn't implement the resolution of CWG1713?

Also, one way to know for sure would be to look at the LLVM bitcode emitted, and confirm that __is_reverse_view<reverse_view<_Tp>> has linkoncde_odr linkage. If so, then clang-tidy would definitely be wrong to flag this.

[• Quuxplusone]

libcxx/include/__iterator/iter_move.h

76

PSA: This will merge-conflict when rebased.

libcxx/include/__ranges/reverse_view.h

117

My understanding is that the variables instantiated from these templates behave the same as any other variables instantiated from templates — they shouldn't need inline for correctness, any more than a variable template (or function template) needs inline for correctness.
However, for consistency with the entire rest of the library, I think it would be a good idea to make these inline constexpr bool throughout. The STL uses inline constexpr bool for its own type-trait variables; even if they had no reason to do that originally, we certainly have no reason to innovate here. (Economist's $100 Bill blog post. :))
So I'm in favor of adding inline.

Also, in __ranges/join_view.h, I would change
constexpr bool __use_const = to static constexpr bool _UseConst = for consistency with some superficially similar code in subrange and reverse_view.

libcxx/include/cstddef

159

Suggest adding a blank line before this one.

[philnik]

libcxx/include/__ranges/reverse_view.h

117

This is now #53519. I'll make a PR later to add inline to the constexpr variables.

libcxx/include/__functional/function.h

 const std::type_info&
 __func<_Fp, _Alloc, _Rp(_A0, _A1, _A2)>::target_type() const
 {
     return typeid(_Fp);
 }
 
 #endif // _LIBCPP_NO_RTTI
 
-}  // __function
+} // namespace __function
 
 template<class _Rp>
 class _LIBCPP_TEMPLATE_VIS function<_Rp()>
 {
     typedef __function::__base<_Rp()> __base;
     aligned_storage<3*sizeof(void*)>::type __buf_;
     __base* __f_;
 

libcxx/include/__iterator/iter_move.h

struct __fn {
 
   // [iterator.cust.move]/1.3
   // Otherwise, ranges::iter_move(E) is ill-formed.
 };
 } // namespace ranges::__iter_move
 
 namespace ranges::inline __cpo {
   inline constexpr auto iter_move = __iter_move::__fn{};
-}
+} // namespace ranges::__cpo
 
 template<__dereferenceable _Tp>
 requires requires(_Tp& __t) { { ranges::iter_move(__t) } -> __referenceable; }
 using iter_rvalue_reference_t = decltype(ranges::iter_move(declval<_Tp&>()));
 
 #endif // !_LIBCPP_HAS_NO_RANGES
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___ITERATOR_ITER_MOVE_H

libcxx/include/__ranges/access.h

struct __fn {
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
       noexcept(noexcept(_LIBCPP_AUTO_CAST(begin(__t))))
     {
       return _LIBCPP_AUTO_CAST(begin(__t));
     }
 
     void operator()(auto&&) const = delete;
   };
-}
+} // namespace __begin
 
 inline namespace __cpo {
   inline constexpr auto begin = __begin::__fn{};
 } // namespace __cpo
 } // namespace ranges
 
 // [range.range]
 
public:
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
       noexcept(noexcept(_LIBCPP_AUTO_CAST(end(__t))))
     {
       return _LIBCPP_AUTO_CAST(end(__t));
     }
 
     void operator()(auto&&) const = delete;
   };
-}
+} // namespace __end
 
 inline namespace __cpo {
   inline constexpr auto end = __end::__fn{};
 } // namespace __cpo
 } // namespace ranges
 
 // [range.access.cbegin]
 
struct __fn {
     template <class _Tp>
       requires is_rvalue_reference_v<_Tp&&>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
     constexpr auto operator()(_Tp&& __t) const
       noexcept(noexcept(ranges::begin(static_cast<const _Tp&&>(__t))))
       -> decltype(      ranges::begin(static_cast<const _Tp&&>(__t)))
       { return          ranges::begin(static_cast<const _Tp&&>(__t)); }
   };
-}
+} // namespace __cbegin
 
 inline namespace __cpo {
   inline constexpr auto cbegin = __cbegin::__fn{};
 } // namespace __cpo
 } // namespace ranges
 
 // [range.access.cend]
 
struct __fn {
     template <class _Tp>
       requires is_rvalue_reference_v<_Tp&&>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
     constexpr auto operator()(_Tp&& __t) const
       noexcept(noexcept(ranges::end(static_cast<const _Tp&&>(__t))))
       -> decltype(      ranges::end(static_cast<const _Tp&&>(__t)))
       { return          ranges::end(static_cast<const _Tp&&>(__t)); }
   };
-}
+} // namespace __cend
 
 inline namespace __cpo {
   inline constexpr auto cend = __cend::__fn{};
 } // namespace __cpo
 } // namespace ranges
 
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___RANGES_ACCESS_H

libcxx/include/__ranges/all.h

template<class _Tp>
                  requires (_Tp&& __t) { ranges::owning_view{_VSTD::forward<_Tp>(__t)}; })
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
     constexpr auto operator()(_Tp&& __t) const
       noexcept(noexcept(ranges::owning_view{_VSTD::forward<_Tp>(__t)}))
     {
       return ranges::owning_view{_VSTD::forward<_Tp>(__t)};
     }
   };
-}
+} // namespace __all
 
 inline namespace __cpo {
   inline constexpr auto all = __all::__fn{};
 } // namespace __cpo
 
 template<ranges::viewable_range _Range>
 using all_t = decltype(views::all(declval<_Range>()));
 
 } // namespace ranges::views
 
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___RANGES_ALL_H

libcxx/include/__ranges/common_view.h

struct __fn : __range_adaptor_closure<__fn> {
 
     template<class _Range>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
     constexpr auto operator()(_Range&& __range) const
       noexcept(noexcept(common_view{_VSTD::forward<_Range>(__range)}))
       -> decltype(      common_view{_VSTD::forward<_Range>(__range)})
       { return          common_view{_VSTD::forward<_Range>(__range)}; }
   };
-}
+} // namespace __common
 
 inline namespace __cpo {
   inline constexpr auto common = __common::__fn{};
-}
+} // namespace __cpo
 } // namespace views
 } // namespace ranges
 
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___RANGES_COMMON_VIEW_H

libcxx/include/__ranges/data.h

struct __fn {
 
     template<__ranges_begin_invocable _Tp>
     _LIBCPP_HIDE_FROM_ABI
     constexpr auto operator()(_Tp&& __t) const
         noexcept(noexcept(_VSTD::to_address(ranges::begin(__t)))) {
       return _VSTD::to_address(ranges::begin(__t));
     }
   };
-}
+} // namespace __data
 
 inline namespace __cpo {
   inline constexpr auto data = __data::__fn{};
 } // namespace __cpo
 } // namespace ranges
 
 // [range.prim.cdata]
 
struct __fn {
     template <class _Tp>
       requires is_rvalue_reference_v<_Tp&&>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
     constexpr auto operator()(_Tp&& __t) const
       noexcept(noexcept(ranges::data(static_cast<const _Tp&&>(__t))))
       -> decltype(      ranges::data(static_cast<const _Tp&&>(__t)))
       { return          ranges::data(static_cast<const _Tp&&>(__t)); }
   };
-}
+} // namespace __cdata
 
 inline namespace __cpo {
   inline constexpr auto cdata = __cdata::__fn{};
 } // namespace __cpo
 } // namespace ranges
 
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___RANGES_DATA_H

libcxx/include/__ranges/empty.h

struct __fn {
     }
 
     template<__can_compare_begin_end _Tp>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool operator()(_Tp&& __t) const
         noexcept(noexcept(bool(ranges::begin(__t) == ranges::end(__t)))) {
       return ranges::begin(__t) == ranges::end(__t);
     }
   };
-}
+} // namespace __empty
 
 inline namespace __cpo {
   inline constexpr auto empty = __empty::__fn{};
 } // namespace __cpo
 } // namespace ranges
 
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___RANGES_EMPTY_H

libcxx/include/__ranges/iota_view.h

constexpr auto operator()(_Start&& __start, _Bound&& __bound) const
       noexcept(noexcept(ranges::iota_view(_VSTD::forward<_Start>(__start), _VSTD::forward<_Bound>(__bound))))
       -> decltype(      ranges::iota_view(_VSTD::forward<_Start>(__start), _VSTD::forward<_Bound>(__bound)))
       { return          ranges::iota_view(_VSTD::forward<_Start>(__start), _VSTD::forward<_Bound>(__bound)); }
   };
 } // namespace __iota
 
 inline namespace __cpo {
   inline constexpr auto iota = __iota::__fn{};
-}
+} // namespace __cpo
 } // namespace views
 } // namespace ranges
 
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___RANGES_IOTA_VIEW_H

libcxx/include/__ranges/reverse_view.h

namespace ranges {
   inline constexpr bool enable_borrowed_range<reverse_view<_Tp>> = enable_borrowed_range<_Tp>;
 
   namespace views {
   namespace __reverse {
     template<class _Tp>
     constexpr bool __is_reverse_view = false;
 
     template<class _Tp>
     constexpr bool __is_reverse_view<reverse_view<_Tp>> = true;
 
     template<class _Tp>
     constexpr bool __is_sized_reverse_subrange = false;
 
     template<class _Iter>
     constexpr bool __is_sized_reverse_subrange<subrange<reverse_iterator<_Iter>, reverse_iterator<_Iter>, subrange_kind::sized>> = true;
 
     template<class _Tp>
struct __fn : __range_adaptor_closure<__fn> {
                   !__is_sized_reverse_subrange<remove_cvref_t<_Range>> &&
                   !__is_unsized_reverse_subrange<remove_cvref_t<_Range>>)
       [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
       constexpr auto operator()(_Range&& __range) const
         noexcept(noexcept(reverse_view{_VSTD::forward<_Range>(__range)}))
         -> decltype(      reverse_view{_VSTD::forward<_Range>(__range)})
         { return          reverse_view{_VSTD::forward<_Range>(__range)}; }
     };
-  }
+  } // namespace __reverse
 
   inline namespace __cpo {
     inline constexpr auto reverse = __reverse::__fn{};
-  }
+  } // namespace __cpo
   } // namespace views
 } // namespace ranges
 
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___RANGES_REVERSE_VIEW_H

libcxx/include/__ranges/size.h

 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if !defined(_LIBCPP_HAS_NO_RANGES)
 
 namespace ranges {
   template<class>
   inline constexpr bool disable_sized_range = false;
-}
+} // namespace ranges
 
 // [range.prim.size]
 
 namespace ranges {
 namespace __size {
   void size(auto&) = delete;
   void size(const auto&) = delete;
 
struct __fn {
     }
 
     template<__difference _Tp>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr __integer_like auto operator()(_Tp&& __t) const
         noexcept(noexcept(ranges::end(__t) - ranges::begin(__t))) {
       return _VSTD::__to_unsigned_like(ranges::end(__t) - ranges::begin(__t));
     }
   };
-}
+} // namespace __size
 
 inline namespace __cpo {
   inline constexpr auto size = __size::__fn{};
 } // namespace __cpo
 } // namespace ranges
 
 // [range.prim.ssize]
 
 namespace ranges {
 namespace __ssize {
   struct __fn {
     template<class _Tp>
       requires requires (_Tp&& __t) { ranges::size(__t); }
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr integral auto operator()(_Tp&& __t) const
       noexcept(noexcept(ranges::size(__t)))
     {
       using _Signed = make_signed_t<decltype(ranges::size(__t))>;
       if constexpr (sizeof(ptrdiff_t) > sizeof(_Signed))
         return static_cast<ptrdiff_t>(ranges::size(__t));
       else
         return static_cast<_Signed>(ranges::size(__t));
     }
   };
-}
+} // namespace __ssize
 
 inline namespace __cpo {
   inline constexpr auto ssize = __ssize::__fn{};
 } // namespace __cpo
 } // namespace ranges
 
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___RANGES_SIZE_H

libcxx/include/__ranges/transform_view.h

struct __fn {
 
     template<class _Fn>
       requires constructible_from<decay_t<_Fn>, _Fn>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
     constexpr auto operator()(_Fn&& __f) const
       noexcept(is_nothrow_constructible_v<decay_t<_Fn>, _Fn>)
     { return __range_adaptor_closure_t(_VSTD::__bind_back(*this, _VSTD::forward<_Fn>(__f))); }
   };
-}
+} // namespace __transform
 
 inline namespace __cpo {
   inline constexpr auto transform = __transform::__fn{};
-}
+} // namespace __cpo
 } // namespace views
 
 } // namespace ranges
 
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___RANGES_TRANSFORM_VIEW_H

libcxx/include/__ranges/view_interface.h

public:
   _LIBCPP_HIDE_FROM_ABI
   constexpr decltype(auto) operator[](range_difference_t<_RARange> __index) const
     noexcept(noexcept(ranges::begin(__derived())[__index]))
   {
     return ranges::begin(__derived())[__index];
   }
 };
 
-}
+} // namespace ranges
 
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___RANGES_VIEW_INTERFACE_H

libcxx/include/cstddef

 template <class _Integer>
   constexpr _EnableByteOverload<_Integer>
   operator>> (byte  __lhs, _Integer __shift) noexcept
   { return static_cast<byte>(static_cast<unsigned char>(static_cast<unsigned int>(__lhs) >> __shift)); }
 
 template <class _Integer, class = _EnableByteOverload<_Integer> >
   _LIBCPP_NODISCARD_EXT constexpr _Integer
   to_integer(byte __b) noexcept { return static_cast<_Integer>(__b); }
-}
+} // namespace std
 
 #endif
 
 #endif // _LIBCPP_CSTDDEF

libcxx/include/variant

 template <class ..._Types>
 using _MakeOverloads _LIBCPP_NODEBUG = typename __make_overloads_imp<
     __make_indices_imp<sizeof...(_Types), 0> >::template _Apply<_Types...>;
 
 template <class _Tp, class... _Types>
 using __best_match_t =
     typename invoke_result_t<_MakeOverloads<_Types...>, _Tp, _Tp>::type;
 
-} // __variant_detail
+} // namespace __variant_detail
 
 template <class... _Types>
 class _LIBCPP_TEMPLATE_VIS variant
     : private __sfinae_ctor_base<
           __all<is_copy_constructible_v<_Types>...>::value,
           __all<is_move_constructible_v<_Types>...>::value>,
       private __sfinae_assign_base<
           __all<(is_copy_constructible_v<_Types> &&