[libcxx] improves diagnostics for containers with bad value types #106296
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Christopher Di Bella (cjdb) Changes
These static_asserts are placed at the very top of each container because they short-circuit further instantiation errors, thereby leading a smaller set of diagnostics for the programmer. Placing them in This commit only cleans up things that are already ill-formed. In particular, Patch is 72.20 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/106296.diff 27 Files Affected:
diff --git a/libcxx/include/__memory/allocator.h b/libcxx/include/__memory/allocator.h
index 0dbdc41d3c3d14..36890f4e1a97d6 100644
--- a/libcxx/include/__memory/allocator.h
+++ b/libcxx/include/__memory/allocator.h
@@ -16,9 +16,12 @@
#include <__memory/allocator_traits.h>
#include <__type_traits/is_const.h>
#include <__type_traits/is_constant_evaluated.h>
+#include <__type_traits/is_function.h>
+#include <__type_traits/is_reference.h>
#include <__type_traits/is_same.h>
#include <__type_traits/is_void.h>
#include <__type_traits/is_volatile.h>
+#include <__type_traits/remove_reference.h>
#include <__utility/forward.h>
#include <cstddef>
#include <new>
@@ -76,8 +79,16 @@ struct __non_trivial_if<true, _Unique> {
template <class _Tp>
class _LIBCPP_TEMPLATE_VIS allocator : private __non_trivial_if<!is_void<_Tp>::value, allocator<_Tp> > {
- static_assert(!is_const<_Tp>::value, "std::allocator does not support const types");
- static_assert(!is_volatile<_Tp>::value, "std::allocator does not support volatile types");
+ static_assert(!is_const<_Tp>::value, "'std::allocator' can only allocate non-const object types");
+ static_assert(!is_volatile<_Tp>::value, "'std::allocator' can only allocate non-volatile object types");
+ static_assert(!is_reference<_Tp>::value || !is_function<typename remove_reference<_Tp>::type>::value,
+ "'std::allocator' can only allocate object types; function references are not objects (consider using "
+ "a function pointer)");
+ static_assert(!is_reference<_Tp>::value,
+ "'std::allocator' can only allocate object types; references are not objects");
+ static_assert(
+ !is_function<_Tp>::value,
+ "'std::allocator' can only allocate object types; functions are not objects (consider using a function pointer)");
public:
typedef size_t size_type;
diff --git a/libcxx/include/array b/libcxx/include/array
index 4db0cb7bd7e3b5..0476f57a86ac24 100644
--- a/libcxx/include/array
+++ b/libcxx/include/array
@@ -127,11 +127,15 @@ template <size_t I, class T, size_t N> const T&& get(const array<T, N>&&) noexce
#include <__type_traits/is_array.h>
#include <__type_traits/is_const.h>
#include <__type_traits/is_constructible.h>
+#include <__type_traits/is_function.h>
#include <__type_traits/is_nothrow_constructible.h>
+#include <__type_traits/is_reference.h>
#include <__type_traits/is_same.h>
#include <__type_traits/is_swappable.h>
#include <__type_traits/is_trivially_relocatable.h>
+#include <__type_traits/is_void.h>
#include <__type_traits/remove_cv.h>
+#include <__type_traits/remove_reference.h>
#include <__utility/empty.h>
#include <__utility/integer_sequence.h>
#include <__utility/move.h>
@@ -167,6 +171,11 @@ _LIBCPP_BEGIN_NAMESPACE_STD
template <class _Tp, size_t _Size>
struct _LIBCPP_TEMPLATE_VIS array {
+ static_assert(!is_reference<_Tp>::value || !is_function<typename remove_reference<_Tp>::type>::value, "'std::array' can only hold object types; function references are not objects (consider using a function pointer)");
+ static_assert(!is_reference<_Tp>::value, "'std::array' can only hold object types; references are not objects");
+ static_assert(!is_function<_Tp>::value, "'std::array' can only hold object types; functions are not objects (consider using a function pointer)");
+ static_assert(!is_void<_Tp>::value, "'std::array' can only hold object types; 'void' is not an object");
+
using __trivially_relocatable = __conditional_t<__libcpp_is_trivially_relocatable<_Tp>::value, array, void>;
// types:
diff --git a/libcxx/include/deque b/libcxx/include/deque
index 759de5d3a030a6..df0ddaebc39e92 100644
--- a/libcxx/include/deque
+++ b/libcxx/include/deque
@@ -212,9 +212,15 @@ template <class T, class Allocator, class Predicate>
#include <__ranges/size.h>
#include <__split_buffer>
#include <__type_traits/is_allocator.h>
+#include <__type_traits/is_const.h>
#include <__type_traits/is_convertible.h>
+#include <__type_traits/is_function.h>
+#include <__type_traits/is_reference.h>
#include <__type_traits/is_same.h>
#include <__type_traits/is_swappable.h>
+#include <__type_traits/is_void.h>
+#include <__type_traits/is_volatile.h>
+#include <__type_traits/remove_reference.h>
#include <__type_traits/type_identity.h>
#include <__utility/forward.h>
#include <__utility/move.h>
@@ -468,6 +474,12 @@ const _DiffType __deque_iterator<_ValueType, _Pointer, _Reference, _MapPointer,
template <class _Tp, class _Allocator /*= allocator<_Tp>*/>
class _LIBCPP_TEMPLATE_VIS deque {
+ static_assert(!is_const<_Tp>::value, "'std::deque' can only hold non-const types");
+ static_assert(!is_volatile<_Tp>::value, "'std::deque' can only hold non-volatile types");
+ static_assert(!is_reference<_Tp>::value || !is_function<typename remove_reference<_Tp>::type>::value, "'std::deque' can only hold object types; function references are not objects (consider using a function pointer)");
+ static_assert(!is_reference<_Tp>::value, "'std::deque' can only hold object types; references are not objects");
+ static_assert(!is_function<_Tp>::value, "'std::deque' can only hold object types; functions are not objects (consider using a function pointer)");
+ static_assert(!is_void<_Tp>::value, "'std::deque' can only hold object types; 'void' is not an object");
public:
// types:
diff --git a/libcxx/include/forward_list b/libcxx/include/forward_list
index b8e3d05588f96e..2b6ad3bc100f90 100644
--- a/libcxx/include/forward_list
+++ b/libcxx/include/forward_list
@@ -220,11 +220,16 @@ template <class T, class Allocator, class Predicate>
#include <__type_traits/conditional.h>
#include <__type_traits/is_allocator.h>
#include <__type_traits/is_const.h>
+#include <__type_traits/is_function.h>
#include <__type_traits/is_nothrow_assignable.h>
#include <__type_traits/is_nothrow_constructible.h>
#include <__type_traits/is_pointer.h>
+#include <__type_traits/is_reference.h>
#include <__type_traits/is_same.h>
#include <__type_traits/is_swappable.h>
+#include <__type_traits/is_void.h>
+#include <__type_traits/is_volatile.h>
+#include <__type_traits/remove_reference.h>
#include <__type_traits/type_identity.h>
#include <__utility/forward.h>
#include <__utility/move.h>
@@ -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");
+ static_assert(!is_volatile<_Tp>::value, "'std::forward_list' can only hold non-volatile types");
+ static_assert(!is_reference<_Tp>::value || !is_function<typename remove_reference<_Tp>::type>::value, "'std::forward_list' can only hold object types; function references are not objects (consider using a function pointer)");
+ static_assert(!is_reference<_Tp>::value, "'std::forward_list' can only hold object types; references are not objects");
+ static_assert(!is_function<_Tp>::value, "'std::forward_list' can only hold object types; functions are not objects (consider using a function pointer)");
+ static_assert(!is_void<_Tp>::value, "'std::forward_list' can only hold object types; 'void' is not an object");
+
typedef __forward_list_base<_Tp, _Alloc> base;
typedef typename base::__node_allocator __node_allocator;
typedef typename base::__node_type __node_type;
diff --git a/libcxx/include/list b/libcxx/include/list
index 929c84de7be449..8da28ff1873d89 100644
--- a/libcxx/include/list
+++ b/libcxx/include/list
@@ -226,10 +226,16 @@ template <class T, class Allocator, class Predicate>
#include <__ranges/from_range.h>
#include <__type_traits/conditional.h>
#include <__type_traits/is_allocator.h>
+#include <__type_traits/is_const.h>
+#include <__type_traits/is_function.h>
#include <__type_traits/is_nothrow_assignable.h>
#include <__type_traits/is_nothrow_constructible.h>
#include <__type_traits/is_pointer.h>
+#include <__type_traits/is_reference.h>
#include <__type_traits/is_same.h>
+#include <__type_traits/is_void.h>
+#include <__type_traits/is_volatile.h>
+#include <__type_traits/remove_reference.h>
#include <__type_traits/type_identity.h>
#include <__utility/forward.h>
#include <__utility/move.h>
@@ -659,8 +665,136 @@ void __list_imp<_Tp, _Alloc>::swap(__list_imp& __c)
__c.__end_.__prev_->__next_ = __c.__end_.__next_->__prev_ = __c.__end_as_link();
}
+// begin-diagnostic-helpers
+// std::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::list must be the one to fire the static_assert.
+// Since std::list inherits from __list_imp, we also need to create specialisations for the rejected types,
+// so that everything appears "good" to std::list (otherwise we'll get lots of unhelpful diagnostics that
+// suppress the one std::list offers).
+template <class _Tp, class _Alloc>
+class _LIBCPP_TEMPLATE_VIS __list_imp<_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 __list_imp<_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 __list_imp<_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 __list_imp<_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 __list_imp<_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 __list_imp<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 list : private __list_imp<_Tp, _Alloc> {
+ static_assert(!is_const<_Tp>::value, "'std::list' can only hold non-const types");
+ static_assert(!is_volatile<_Tp>::value, "'std::list' can only hold non-volatile types");
+ static_assert(!is_reference<_Tp>::value || !is_function<typename remove_reference<_Tp>::type>::value, "'std::list' can only hold object types; function references are not objects (consider using a function pointer)");
+ static_assert(!is_reference<_Tp>::value, "'std::list' can only hold object types; references are not objects");
+ static_assert(!is_function<_Tp>::value, "'std::list' can only hold object types; functions are not objects (consider using a function pointer)");
+ static_assert(!is_void<_Tp>::value, "'std::list' can only hold object types; 'void' is not an object");
+
typedef __list_imp<_Tp, _Alloc> base;
typedef typename base::__node_type __node_type;
typedef typename base::__node_allocator __node_allocator;
diff --git a/libcxx/include/map b/libcxx/include/map
index 02bd17ccb4e8cb..3a801c0162a3f9 100644
--- a/libcxx/include/map
+++ b/libcxx/include/map
@@ -592,6 +592,9 @@ erase_if(multimap<Key, T, Compare, Allocator>& c, Predicate pred); // C++20
#include <__ranges/from_range.h>
#include <__tree>
#include <__type_traits/is_allocator.h>
+#include <__type_traits/is_function.h>
+#include <__type_traits/is_lvalue_reference.h>
+#include <__type_traits/is_void.h>
#include <__utility/forward.h>
#include <__utility/piecewise_construct.h>
#include <__utility/swap.h>
@@ -962,6 +965,13 @@ public:
template <class _Key, class _Tp, class _Compare = less<_Key>, class _Allocator = allocator<pair<const _Key, _Tp> > >
class _LIBCPP_TEMPLATE_VIS map {
+ static_assert(!is_lvalue_reference<_Key>::value || !is_function<typename remove_reference<_Key>::type>::value, "'std::map::key_type' can only hold object types; function references are not objects (consider using a function pointer)");
+ static_assert(!is_lvalue_reference<_Key>::value, "'std::map::key_type' can only hold object types; references are not objects");
+ static_assert(!is_function<_Key>::value, "'std::map::key_type' can only hold object types; functions are not objects (consider using a function pointer)");
+ static_assert(!is_void<_Key>::value, "'std::map::key_type' can only hold object types; 'void' is not an object");
+
+ static_assert(!is_function<_Tp>::value, "'std::map::mapped_type' can only hold object or reference types; functions are neither (consider using a function pointer or reference)");
+ static_assert(!is_void<_Tp>::value, "'std::map::mapped_type' can only hold object types; 'void' is not an object");
public:
// types:
typedef _Key key_type;
@@ -1639,6 +1649,13 @@ erase_if(map<_Key, _Tp, _Compare, _Allocator>& __c, _Predicate __pred) {
template <class _Key, class _Tp, class _Compare = less<_Key>, class _Allocator = allocator<pair<const _Key, _Tp> > >
class _LIBCPP_TEMPLATE_VIS multimap {
+ static_assert(!is_lvalue_reference<_Key>::value || !is_function<typename remove_reference<_Key>::type>::value, "'std::multimap::key_type' can only hold object types; function references are not objects (consider using a function pointer)");
+ static_assert(!is_lvalue_reference<_Key>::value, "'std::multimap::key_type' can only hold object types; references are not objects");
+ static_assert(!is_function<_Key>::value, "'std::multimap::key_type' can only hold object types; functions are not objects (consider using a function pointer)");
+ static_assert(!is_void<_Key>::value, "'std::multimap::key_type' can only hold object types; 'void' is not an object");
+
+ static_assert(!is_function<_Tp>::value, "'std::multimap::mapped_type' can only hold object or reference types; functions are neither (consider using a function pointer or reference)");
+ static_assert(!is_void<_Tp>::value, "'std::multimap::mapped_type' can only hold object types; 'void' is not an object");
public:
// types:
typedef _Key key_type;
diff --git a/libcxx/include/set b/libcxx/include/set
index 94533583798699..c5c1f664283648 100644
--- a/libcxx/include/set
+++ b/libcxx/include/set
@@ -531,6 +531,12 @@ erase_if(multi...
[truncated]
|
|
The motivation for this patch comes from routinely getting downstream user bugs about the helpfulness of the current diagnostics. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
`std::vector<int&>` generates nearly 170 lines of diagnostic, most of which are redundant, and it's only helpful if you are already familiar with the rule that containers can't hold reference types. This commit reduces it to only a handful of lines, all of which are dedicated to clearly communicating the problem. It also applies this to all the other containers, and for all non-cv-qualified and non-object types. These static_asserts are placed at the very top of each container because they short-circuit further instantiation errors, thereby leading a smaller set of diagnostics for the programmer. Placing them in `std::allocator` (which is the common denominator for all containers) doesn't do the short circuiting, and we thus end up with several unhelpful diagnostics after the helpful one. This commit only cleans up things that are already diagnosed as compile-time errors. In particular, `map<int&&, int>` should be ill-formed, per [associative.reqmts.general]/p8. libc++ and libstdc++ currently permit that, but not `map<int&, int>`. As such, this commit only adds a static assert for `map<K&, V>`, and defers diagnosing valid productions that should be ill-formed to a future commit.
cjdb
force-pushed
the
cleaner-libcxx-diagnostics
branch
from
59e11c6
to
5529499
Compare
libcxx/include/__memory/allocator.h
Outdated
| static_assert(!is_const<_Tp>::value, "'std::allocator' can only allocate non-const object types"); | ||
| static_assert(!is_volatile<_Tp>::value, "'std::allocator' can only allocate non-volatile object types"); | ||
| static_assert(!is_reference<_Tp>::value || !is_function<typename remove_reference<_Tp>::type>::value, | ||
| "'std::allocator' can only allocate object types; function references are not objects (consider using " | ||
| "a function pointer)"); | ||
| static_assert(!is_reference<_Tp>::value, | ||
| "'std::allocator' can only allocate object types; references are not objects"); |
There was a problem hiding this comment.
Could we just check is_same<remove_cvref_t<_Tp>, _Tp> instead?
There was a problem hiding this comment.
That erases the tailored diagnostics; noting references are not objects, etc., are hints I that believe significantly improves UX.
There was a problem hiding this comment.
Folks internally suggested that I change the text to the following, as they feel it'll be more helpful than what I'm currently proposing.
std::allocator<T const>:'std::allocator' cannot allocate const objects(return to status quo)std::allocator<T volatile>:'std::allocator' cannot allocate volatile objects(return to status quo)std::allocator<T&>:'std::allocator' cannot allocate referencesstd::allocator<T()>:'std::allocator' cannot allocate functions
We do lose the function reference suggestion, but I'm okay with doing that for something that's more helpful.
There was a problem hiding this comment.
Why not simply "cannot allocate cv or reference qualified objects"? I'm not sure how much of an improvement it is to specify the qualifier explicitly, since it's already shown in the diagnostic.
There was a problem hiding this comment.
I kicked the internal discussion off by asking if the average C++ programmer would be able to understand 'std::vector' can only allocate non-cv-qualified object types, and the feedback I've received can be aggregated as roughly
- most C++ programmers don't know what the term "cv-qualified" means
- we should only mention relevant information, and mentioning "volatile" when
volatileisn't used was repeatedly called out in particular - most C++ programmers probably aren't going to know that "object" has a very different meaning to other languages
- most non-toolchain folks in the discussion seemed to prefer separate messages; toolchain folks were sort of ambivalent
What do you feel a single check will offer over independent checks?
There was a problem hiding this comment.
What is your concern with respect to the number of instantiations?
There was a problem hiding this comment.
Well, I'm concerned that we instantiate a bunch of types for the rare case they are actually required for diagnostic purposes. Every instantiation takes time and memory, and for a type that is used everywhere, like vector, we should keep unnecessary instantiations as low as possible.
There was a problem hiding this comment.
It sounds to me like you're concerned about build times. Have you run any preliminary analysis to determine that there is a noticeable impact?
There was a problem hiding this comment.
This is more a death by a thousand cuts situation than something significant I think. If we add this many checks everywhere people will definitely notice, but not if we just add it in a single place.
There was a problem hiding this comment.
Very good point, I agree. I think I've got something that bridges the gap between what you suggested above, and what I was originally hoping to land.
I think the best way to enact `git clang-format` might be to use `git clang-format <hash-before-your-commit-first>` instead of `git clang-format HEAD~n`. This ensures that all commits in a PR are formatted, instead of just the last n (which one might miscount, like I probably did).
The trick is to run ``` $ git-clang-format --diff <base-hash> --extensions ,cpp,h | patch -p1 ```
libcxx/include/__memory/allocator.h
Outdated
| static_assert(!is_const<_Tp>::value, "'std::allocator' cannot allocate const types"); | ||
| static_assert(!is_volatile<_Tp>::value, "'std::allocator' cannot allocate volatile types"); | ||
| static_assert(!is_reference<_Tp>::value, "'std::allocator' cannot allocate references"); | ||
| static_assert(!is_function<_Tp>::value, "'std::allocator' cannot allocate functions"); |
There was a problem hiding this comment.
Can we tie these checks back to the Standard wording that says so? It doesn't have to be in the static assert message, but at least in a comment.
libcxx/test/libcxx/containers/associative/multimap/non_cv_object.verify.cpp
Outdated
Show resolved
Hide resolved
|
|
A Clang pragma that lets us "hide" macro expansion from diagnostics so that users don't need to read implementation details would be a good thing to explore in the future.
| // allocator that meets the Cpp17Allocator requirements (https://eel.is/c++draft/allocator.requirements). | ||
| // In particular, this means that containers should only accept non-cv-qualified object types, and | ||
| // types that are Cpp17Erasable. | ||
| #define _LIBCPP_CHECK_CONTAINER_VALUE_TYPE_REQUIREMENTS_BASE(_Container, _Tp) \ |
There was a problem hiding this comment.
I don't see this macro being used outside of the definition of _LIBCPP_CHECK_CONTAINER_VALUE_TYPE_REQUIREMENTS_FULL -- is that an oversight or should this be inlined into _LIBCPP_CHECK_CONTAINER_VALUE_TYPE_REQUIREMENTS_FULL?
There was a problem hiding this comment.
I think I was experimenting to identify the bare minimum for all containers, and possibly forgot about that due to the (long) weekend.
There was a problem hiding this comment.
Ah! It was an attempt to share the content with std::allocator, not to handle std::array and the map types (the latter of which will be converted after the clean-up).
Co-authored-by: Louis Dionne <ldionne.2@gmail.com>
`std::llocator` apparently supports unbounded arrays prior to C++20, so we need to change the `is_array` check to only check for bounded arrays.
|
|
||
| #define _LIBCPP_CHECK_CONTAINER_VALUE_TYPE_REQUIREMENTS(_Container, _Tp) \ | ||
| static_assert( \ | ||
| !__libcpp_is_unbounded_array<_Tp>::value, "'std::" _Container "' cannot hold C arrays of an unknown size"); \ |
There was a problem hiding this comment.
Instead of passing vector as the _Container argument, I'd pass std::vector and simplify the message here. The message would become _Container " cannot hold C arrays of an unknown size".
There was a problem hiding this comment.
Hmm, I'd prefer to keep the single quotes so that it's consistent with how Clang outputs type information. For example, this is the diagnostic we get for this erroneous code:
namespace std {
template<class T>
struct S1;
template<class T>
struct S2;
}
std::S1<std::S2> y;error: use of class template 'std::S2' requires template arguments
This helps us remain more consistent as a whole implementation:
error: static assertion failed: 'std::vector' cannot hold C arrays of an unknown size
WDYT?
| export std_private_type_traits_diagnostic_utilities | ||
| export std_private_type_traits_is_bounded_array | ||
| export std_private_type_traits_is_const | ||
| export std_private_type_traits_is_function | ||
| export std_private_type_traits_is_reference | ||
| export std_private_type_traits_is_unbounded_array | ||
| export std_private_type_traits_is_void | ||
| export std_private_type_traits_is_volatile |
There was a problem hiding this comment.
Did you run into CI issues if you didn't do that? I don't think these need to be exported from the module since they are only being used in the implementation.
There was a problem hiding this comment.
Yeah, I'm puzzled too. Our CI build script wasn't happy when building the modules. Might be worth trying on your end to see if I'm doing something silly?
libcxx/include/unordered_map
Outdated
| // TODO(#106635): replace with _LIBCPP_CHECK_CONTAINER_VALUE_TYPE_REQUIREMENTS | ||
| // Remember to remove relevant headers when this is completed. |
There was a problem hiding this comment.
I read the description of microsoft/STL#3660 and it looks like the Standard does allow these types to be used. If that's the case, then #106635 should be closed until we have a LWG issue or a paper that changes the Standard, and these TODOs should be removed since we won't be able to address them.
Note that I have no love for reference types in these locations, but I also want to avoid adding non-actionable TODOs.
There was a problem hiding this comment.
I think MS/STL is allowing these as an extension? I see that the PR mentions that the standard permits this, but my interpretation of the linked wording from #106635 already prohibits unordered_map<int, int&>. I've asked for input in microsoft/STL#3660.
Nonetheless, we weren't catching it before, and have been Hyrum's Law'd.
There was a problem hiding this comment.
Okay, I get what's happening, thanks to cplusplus/draft#7249. I've parked #106635, and have replaced the TODOs with // not an error.
There was a problem hiding this comment.
Per #106635 (comment), I've reopened the issue, and have reverted bb82c95.
There was a problem hiding this comment.
So IIUC basically references in these locations are not permitted, and we should clean up the usage.
There was a problem hiding this comment.
References, const, volatile, etc. I'm especially leery of map<int const, int>, because it's in no way interface-distinguishable from map<int, int>, yet is its own instantiation.
I'm happy to take point on that over the next few releases, but we'll need to come up with a plan (and potentially coordinate with MS/STL and libstdc++).
Nikolas pointed out that checking all the traits in all cases will probably grind builds to a halt as we improve our diagnostics, so this commit makes a point of reducing the number of instantiations as much as possible, and preserves the highly specific messages.
| struct __bounded_arrays_allowed_only_after_cxx20 : integral_constant<bool, __libcpp_is_bounded_array<_Tp>::value> {}; | ||
| #endif | ||
|
|
||
| #define _LIBCPP_CHECK_ALLOCATOR_VALUE_TYPE_REQUIREMENTS(_Template, _Verb) \ |
There was a problem hiding this comment.
We don't have to _Uglify macro parameters.
Co-authored-by: Nikolas Klauser <nikolasklauser@berlin.de>
Co-authored-by: Nikolas Klauser <nikolasklauser@berlin.de>
|
|
||
| #define _LIBCPP_DEFINE__REQUIRES_CV_UNQUALIFIED_OBJECT_TYPE(_Template, _Verb) \ | ||
| template <class _Tp> \ | ||
| struct __requires_cv_unqualified_object_type<_Template, _Tp, false> \ |
There was a problem hiding this comment.
Is there a reason why the patch now defines these __requires_cv_unqualified_object_type specializations for each container? Was that in response to the comment about too many instantiations?
This adds complexity but I don't see the benefit (yet), I'd rather keep this patch as simple as can be. After all the goal here is to issue diagnostics -- that's a fairly simple problem and I'd like to keep the solution accordingly simple.
There was a problem hiding this comment.
Is there a reason why the patch now defines these
__requires_cv_unqualified_object_typespecializations for each container? Was that in response to the comment about too many instantiations?
Yes. I ended up benchmarking Chrome's build times and while we could probably tolerate this patch in isolation, stacking similar changes will eventually tank build times.
This adds complexity but I don't see the benefit (yet), I'd rather keep this patch as simple as can be. After all the goal here is to issue diagnostics -- that's a fairly simple problem and I'd like to keep the solution accordingly simple.
Issuing simple diagnostics is simple. The goal of this patch is to issue diagnostics that are informative to the reader, using language that they're likely to understand; achieving that without adversely impacting build times is apparently more challenging to simplify.
Could you help me understand what it is about the complexity that raises concern, please?
There was a problem hiding this comment.
Basically, I don't understand why the macros don't expand to a bunch of static_asserts directly (like it used to do), instead of going through an additional partial specialization of __requires_cv_unqualified_object_type & friends. That's a complex mechanism for doing basically static_assert(condition) and I don't understand what we're gaining from that complexity. In fact, I would expect this approach to have more impact on build times since we instantiate more stuff and we have a partial specialization.
There was a problem hiding this comment.
Ah, I get where you're coming from now, thank you. This is a metaprogramming hack to preserve performance for valid use.
- In the case where
decay_t<_Tp>matches_Tp:__requires_cv_unqualified_typeshould only need to instantiate two templates:decay_t<_Tp>and__requires_cv_unqualified_type<_Template, _Tp, true>. - In case where
decay_t<_Tp>is different to_Tp:__requires_cv_unqualified_typethen instantiates all the checks to see why that isn't the case. This is indeed slower, but we're now going for a failed build, and tooling tends to burn compile times in favour of helpful diagnostics.
This is in contrast to a raw macro, which performs all the checks, regardless of validity (which cakes on time as you use more unique container templates, to the point of being noticeable).
Neither __allocator_requirements, nor __container_requirements have the same effect, so I'm happy to revert those without further convincing.
Chromium is a good candidate for benchmarking the worst-case scenario, because it has well over 100k utterances of std::vector. Would providing benchmarks be helpful? (It'll take ~a day to get those as I discarded the results from last week, and I tend to run quite a few trials to reduce the impact of noise.)
std::vector<int&>generates nearly 170 lines of diagnostic, most of which are redundant, and it's only helpful if you are already familiar with the rule that containers can't hold reference types. This commit reduces it to only a handful of lines, all of which are dedicated to clearly communicating the problem. It also applies this to all the other containers, and for all non-cv-qualified and non-object types.These static_asserts are placed at the very top of each container because they short-circuit further instantiation errors, thereby leading a smaller set of diagnostics for the programmer. Placing them in
std::allocator(which is the common denominator for all containers) doesn't do the short circuiting, and we thus end up with several unhelpful diagnostics after the helpful one.This commit only cleans up things that are already diagnosed as compile-time errors. In particular,
std::map<int&&, int>should be ill-formed, per [associative.reqmts.general]/p8. libc++ and libstdc++ currently permit that, but notstd::map<int&, int>. As such, this commit only adds a static assert formap<K&, V>, and defers diagnosing valid productions that should be ill-formed to a future commit.