-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[libcxx] improves diagnostics for containers with bad value types #106296
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
base: main
Are you sure you want to change the base?
Changes from 30 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
20e0996
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 |
|---|---|---|
|
|
@@ -1498,7 +1498,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 | ||
|
||
| } | ||
| 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" } | ||
|
|
@@ -1861,6 +1871,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_typespecializations 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_assertsdirectly (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.Uh oh!
There was an error while loading. Please reload this page.
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_typeshould 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_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_requirementshave 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.)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.
Ping
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.
Ping :)
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.
I'm skeptical about the performance claims since the current solution requires instantiating
is_same,__decay_t,__requires_cv_unqualified_object_typeand 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_vversions of these traits (which would bump requirements to C++17), or we could use the compiler builtins directly for__is_referenceand__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_asserts seems really difficult to justify.I would have no opposition to this patch if it was simplified to something like:
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_asserts 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).