[libcxx-commits] [libcxx] [libcxx] improves diagnostics for containers with bad value types (PR #106296)

[Louis Dionne via libcxx-commits]

================
@@ -635,8 +640,136 @@ void __forward_list_base<_Tp, _Alloc>::clear() _NOEXCEPT {
   __before_begin()->__next_ = nullptr;
 }
 
+// begin-diagnostic-helpers
+// std::forward_list can only have non-cv-qualified object types as its value type, which we diagnose.
+// In order to short-cirucit redundant (and cryptic) diagnostics, std::forward_list must be the one
+// to fire the static_assert. Since std::forward_list inherits from __forward_list_base, we also need
+// to create specialisations for the rejected types, so that everything appears "good" to std::forward_list
+// (otherwise we'll get lots of unhelpful diagnostics that suppress the one std::forward_list offers).
+template <class _Tp, class _Alloc>
+class _LIBCPP_TEMPLATE_VIS __forward_list_base<_Tp const, _Alloc> {
+public:
+  using __node_allocator = void*;
+  using __node_alloc_traits = void*;
+  using __node_pointer = void*;
+  using __node_type = void*;
+  using __node_base = void*;
+  using __node_base_pointer = void*;
+  using __link_pointer = void*;
+
+  using pointer = int*;
+  using const_pointer = int const*;
+  using size_type = unsigned int;
+  using difference_type = int;
+  using iterator = int*;
+  using const_iterator = int const*;
+};
+
+template <class _Tp, class _Alloc>
+class _LIBCPP_TEMPLATE_VIS __forward_list_base<_Tp volatile, _Alloc> {
+public:
+  using __node_allocator = void*;
+  using __node_alloc_traits = void*;
+  using __node_pointer = void*;
+  using __node_type = void*;
+  using __node_base = void*;
+  using __node_base_pointer = void*;
+  using __link_pointer = void*;
+
+  using pointer = int*;
+  using const_pointer = int const*;
+  using size_type = unsigned int;
+  using difference_type = int;
+  using iterator = int*;
+  using const_iterator = int const*;
+};
+
+template <class _Tp, class _Alloc>
+class _LIBCPP_TEMPLATE_VIS __forward_list_base<_Tp&, _Alloc> {
+public:
+  using __node_allocator = void*;
+  using __node_alloc_traits = void*;
+  using __node_pointer = void*;
+  using __node_type = void*;
+  using __node_base = void*;
+  using __node_base_pointer = void*;
+  using __link_pointer = void*;
+
+  using pointer = int*;
+  using const_pointer = int const*;
+  using size_type = unsigned int;
+  using difference_type = int;
+  using iterator = int*;
+  using const_iterator = int const*;
+};
+
+template <class _Tp, class _Alloc>
+class _LIBCPP_TEMPLATE_VIS __forward_list_base<_Tp&&, _Alloc> {
+public:
+  using __node_allocator = void*;
+  using __node_alloc_traits = void*;
+  using __node_pointer = void*;
+  using __node_type = void*;
+  using __node_base = void*;
+  using __node_base_pointer = void*;
+  using __link_pointer = void*;
+
+  using pointer = int*;
+  using const_pointer = int const*;
+  using size_type = unsigned int;
+  using difference_type = int;
+  using iterator = int*;
+  using const_iterator = int const*;
+};
+
+template <class _Tp, class... _Args, class _Alloc>
+class _LIBCPP_TEMPLATE_VIS __forward_list_base<_Tp(_Args...), _Alloc> {
+public:
+  using __node_allocator = void*;
+  using __node_alloc_traits = void*;
+  using __node_pointer = void*;
+  using __node_type = void*;
+  using __node_base = void*;
+  using __node_base_pointer = void*;
+  using __link_pointer = void*;
+
+  using pointer = int*;
+  using const_pointer = int const*;
+  using size_type = unsigned int;
+  using difference_type = int;
+  using iterator = int*;
+  using const_iterator = int const*;
+};
+
+template <class _Alloc>
+class _LIBCPP_TEMPLATE_VIS __forward_list_base<void, _Alloc> {
+public:
+  using __node_allocator = void*;
+  using __node_alloc_traits = void*;
+  using __node_pointer = void*;
+  using __node_type = void*;
+  using __node_base = void*;
+  using __node_base_pointer = void*;
+  using __link_pointer = void*;
+
+  using pointer = int*;
+  using const_pointer = int const*;
+  using size_type = unsigned int;
+  using difference_type = int;
+  using iterator = int*;
+  using const_iterator = int const*;
+};
+// end-diagnostic-helpers
+
 template <class _Tp, class _Alloc /*= allocator<_Tp>*/>
 class _LIBCPP_TEMPLATE_VIS forward_list : private __forward_list_base<_Tp, _Alloc> {
+  static_assert(!is_const<_Tp>::value, "'std::forward_list' can only hold non-const types");
----------------
ldionne wrote:

While I think the `static_assert`s definitely make sense (in some form, maybe grouped under a common utility), I really don't think the added complexity is worth it for the `__forward_list_base` specializations above.

What is the reason for needing those? If the reason is that the compiler otherwise won't skip emitting spurious diagnostics, IMO I would treat it as a compiler problem. In particular, we could inherit from `__forward_list_value_type_constraints<_Tp>` and have the `static_assert`s in there, and somehow have Clang stop trying to compile junk when it encounters a hard error like a failed `static_assert` (I never really understood why it tries to keep going).

https://github.com/llvm/llvm-project/pull/106296