-
Notifications
You must be signed in to change notification settings - Fork 11.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[libcxx] improves diagnostics for containers with bad value types #106296
base: main
Are you sure you want to change the base?
Changes from all commits
5529499
1e6b81b
6c1e1e3
938299d
65346a3
472ffcf
7596a2e
25e999f
5a1b902
99a4509
45c85d6
dbf872a
e596964
e2d65c0
0487281
6d9ce9e
c49b7e8
eb2fd68
55a2e7a
d9043c4
ab41024
7bd03b3
ef9d9c3
f0f88d7
bb82c95
33a452d
6981a2a
9fe3f4a
b97942e
02ffa16
f75d1ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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> \ | ||
: integral_constant<bool, \ | ||
!(is_const<_Tp>::value || is_volatile<_Tp>::value || is_reference<_Tp>::value || \ | ||
is_function<_Tp>::value)> { \ | ||
static_assert(!is_const<_Tp>::value, "'std::" #_Template "' cannot " _Verb " const types"); \ | ||
static_assert(!is_volatile<_Tp>::value, "'std::" #_Template "' cannot " _Verb " volatile types"); \ | ||
static_assert(!is_reference<_Tp>::value, "'std::" #_Template "' cannot " _Verb " references"); \ | ||
static_assert(!is_function<_Tp>::value, "'std::" #_Template "' cannot " _Verb " functions"); \ | ||
} | ||
|
||
// Per https://eel.is/c++draft/containers#container.reqmts-64, allocator-aware containers must have an | ||
// 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. | ||
template <template <class...> class _Template, class _Tp, bool = is_same<__decay_t<_Tp>, _Tp>::value> | ||
struct __allocator_requirements : true_type {}; | ||
|
||
#if _LIBCPP_STD_VER >= 20 | ||
template <class _Tp> | ||
inline const bool __bounded_arrays_allowed_only_after_cxx20 = false; | ||
#else | ||
template <class _Tp> | ||
inline const bool __bounded_arrays_allowed_only_after_cxx20 = __libcpp_is_bounded_array<_Tp>::value; | ||
#endif | ||
|
||
#define _LIBCPP_DEFINE__ALLOCATOR_VALUE_TYPE_REQUIREMENTS(_Template, _Verb) \ | ||
_LIBCPP_DEFINE__REQUIRES_CV_UNQUALIFIED_OBJECT_TYPE(_Template, _Verb); \ | ||
template <class _Tp> \ | ||
struct __allocator_requirements<_Template, _Tp, false> \ | ||
: integral_constant<bool, \ | ||
__requires_cv_unqualified_object_type<_Template, _Tp>::value && \ | ||
!__bounded_arrays_allowed_only_after_cxx20<_Tp> > { \ | ||
static_assert(!__bounded_arrays_allowed_only_after_cxx20<_Tp>, \ | ||
"'std::" #_Template "' cannot " _Verb " C arrays before C++20"); \ | ||
} | ||
|
||
template <template <class...> class, class> | ||
struct __container_requirements : false_type { | ||
static_assert( | ||
false, | ||
"a new container has been defined; please define '_LIBCPP_DEFINE__CONTAINER_VALUE_TYPE_REQUIREMENTS' for " | ||
"that container"); | ||
}; | ||
|
||
#define _LIBCPP_DEFINE__CONTAINER_VALUE_TYPE_REQUIREMENTS(_Template) \ | ||
_LIBCPP_DEFINE__ALLOCATOR_VALUE_TYPE_REQUIREMENTS(_Template, "hold"); \ | ||
template <class _Tp> \ | ||
struct __container_requirements<_Template, _Tp> \ | ||
: integral_constant<bool, \ | ||
__allocator_requirements<_Template, _Tp>::value && \ | ||
!(is_void<_Tp>::value || __libcpp_is_unbounded_array<_Tp>::value)> { \ | ||
static_assert(!is_void<_Tp>::value, "'std::" #_Template "' cannot hold 'void'"); \ | ||
static_assert(!__libcpp_is_unbounded_array<_Tp>::value, \ | ||
"'std::" #_Template "' cannot hold C arrays of an unknown size"); \ | ||
} | ||
|
||
_LIBCPP_END_NAMESPACE_STD | ||
|
||
#endif // _LIBCPP___TYPE_TRAITS_DIAGNOSTIC_UTILITIES_H |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1494,7 +1494,17 @@ module std_private_memory_align [system] { header "__m | |
module std_private_memory_aligned_alloc [system] { header "__memory/aligned_alloc.h" } | ||
module std_private_memory_allocate_at_least [system] { header "__memory/allocate_at_least.h" } | ||
module std_private_memory_allocation_guard [system] { header "__memory/allocation_guard.h" } | ||
module std_private_memory_allocator [system] { header "__memory/allocator.h" } | ||
module std_private_memory_allocator [system] { | ||
header "__memory/allocator.h" | ||
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 | ||
Comment on lines
+1499
to
+1506
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
} | ||
module std_private_memory_allocator_arg_t [system] { header "__memory/allocator_arg_t.h" } | ||
module std_private_memory_allocator_destructor [system] { header "__memory/allocator_destructor.h" } | ||
module std_private_memory_allocator_traits [system] { header "__memory/allocator_traits.h" } | ||
|
@@ -1871,6 +1881,10 @@ module std_private_type_traits_decay [system | |
} | ||
module std_private_type_traits_dependent_type [system] { header "__type_traits/dependent_type.h" } | ||
module std_private_type_traits_desugars_to [system] { header "__type_traits/desugars_to.h" } | ||
module std_private_type_traits_diagnostic_utilities [system] { | ||
textual header "__type_traits/diagnostic_utilities.h" | ||
export * | ||
} | ||
module std_private_type_traits_disjunction [system] { header "__type_traits/disjunction.h" } | ||
module std_private_type_traits_enable_if [system] { header "__type_traits/enable_if.h" } | ||
module std_private_type_traits_extent [system] { header "__type_traits/extent.h" } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 basicallystatic_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I get where you're coming from now, thank you. This is a metaprogramming hack to preserve performance for valid use.
decay_t<_Tp>
matches_Tp
:__requires_cv_unqualified_type
should only need to instantiate two templates:decay_t<_Tp>
and__requires_cv_unqualified_type<_Template, _Tp, true>
.decay_t<_Tp>
is different to_Tp
:__requires_cv_unqualified_type
then 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.)