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

[Louis Dionne via llvm-commits]

================
@@ -0,0 +1,94 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___TYPE_TRAITS_DIAGNOSTIC_UTILITIES_H
+#define _LIBCPP___TYPE_TRAITS_DIAGNOSTIC_UTILITIES_H
+
+#include <__config>
+#include <__type_traits/decay.h>
+#include <__type_traits/integral_constant.h>
+#include <__type_traits/is_bounded_array.h>
+#include <__type_traits/is_const.h>
+#include <__type_traits/is_function.h>
+#include <__type_traits/is_reference.h>
+#include <__type_traits/is_same.h>
+#include <__type_traits/is_unbounded_array.h>
+#include <__type_traits/is_void.h>
+#include <__type_traits/is_volatile.h>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#  pragma GCC system_header
+#endif
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+// Many templates require their type parameters to be cv-unqualified objects.
+template <template <class...> class _Template, class _Tp, bool = is_same<__decay_t<_Tp>, _Tp>::value>
+struct __requires_cv_unqualified_object_type : true_type {};
+
+#define _LIBCPP_DEFINE__REQUIRES_CV_UNQUALIFIED_OBJECT_TYPE(_Template, _Verb)                                          \
+  template <class _Tp>                                                                                                 \
+  struct __requires_cv_unqualified_object_type<_Template, _Tp, false>                                                  \
----------------
ldionne wrote:

I'm skeptical about the performance claims since the current solution requires instantiating `is_same`, `__decay_t`, `__requires_cv_unqualified_object_type` and finding the right specialization for it. That doesn't seem to be significantly less expensive than instantiating the 4 traits directly. And if we wanted to be even speedier, we could use the `_v` versions of these traits (which would bump requirements to C++17), or we could use the compiler builtins directly for `__is_reference` and `__is_function`, which we use unconditionally.

And even if there's a very very small compile-time hit, the complexity introduced by the current approach for what is essentially 4 `static_assert`s seems really difficult to justify.

I would have no opposition to this patch if it was simplified to something like:

```c++
#if _LIBCPP_STD_VER >= 20
# define _LIBCPP_CHECK_ALLOCATOR_REQUIREMENTS(template, value_type)                                                   \
    static_assert(!is_const<value_type>::value, #template " cannot have a const value_type");                         \
    static_assert(!is_volatile<value_type>::value, #template " cannot have a volatile value_type");                   \
    static_assert(!is_reference<value_type>::value, #template " cannot have a value_type that is a reference type");  \
    static_assert(!is_function<value_type>::value, #template " cannot have a value_type that is a function type")
#else
# define _LIBCPP_CHECK_ALLOCATOR_REQUIREMENTS(template, value_type)                                                   \
    static_assert(!is_const<value_type>::value, #template " cannot have a const value_type");                         \
    static_assert(!is_volatile<value_type>::value, #template " cannot have a volatile value_type");                   \
    static_assert(!is_reference<value_type>::value, #template " cannot have a value_type that is a reference type");  \
    static_assert(!is_function<value_type>::value, #template " cannot have a value_type that is a function type");    \
    static_assert(!__libcpp_is_bounded_array<value_type>::value, #template " cannot have a value_type that is a bounded array before C++20")
#endif

#define _LIBCPP_CHECK_ALLOCATOR_AWARE_CONTAINER_REQUIREMENTS(template, value_type)                                    \
  _LIBCPP_CHECK_ALLOCATOR_REQUIREMENTS(template, value_type);                                                         \
  static_assert(!is_void<value_type>::value, #template " cannot hold void");                                          \
  static_assert(!__libcpp_is_unbounded_array<value_type>::value, #template " cannot hold C arrays of an unknown size")
```

I think it's difficult to justify adding more complexity than that either for compilation times or for diagnostics quality. After all, this kind of error is something that users will see once when they try to start using a container wrong, and they will very likely see this near the top of their call stack (i.e. this is unlikely to trigger deep down inside a dependent library). 95% of the benefit of this patch is obtained by adding the `static_assert`s in the simplest way possible, but as it stands a large part of the complexity comes from trying to go the last 5% in terms of diagnostics quality and compilation time (which I'm still skeptical of). I don't think that's the right tradeoff, and that's the source of my pushback.

So, TLDR: I think adding these diagnostics is great and I love that we're making the library more robust to misuse and improving the diagnostics quality. But I don't think the patch achieves the right complexity tradeoff in its current state (it started more naive and with a better tradeoff IMO).

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