[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:

The best diagnostics seem to be the ones we have when we put the checks in `forward_list` and use `-ferror-limit=1`, basically. I've very often thought to myself that the diagnostics quality would be greatly improved if we just had `-ferror-limit=1` be the default. It's a very common source of issues when using libraries and I more often than not wish the compiler stopped.

@AaronBallman Do you think Clang would be open to considering a change of this kind?

@cjdb As I said before, I think the static assertions are fine, however I don't think trying to work around `-ferror-limit` is reasonable. It's unfortunately an infinitely deep can of worms since basically every place where we have a `static_assert` has the exact same problem. Furthermore, as Clang changes (or with different compilers), our attempts to silence spurious diagnostics could end up not working or simply producing different diagnostics -- basically I think that's a very brittle construct to maintain.

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