From b6572b21a7b8351b6fb44e4a07ee7590b0ab346c Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Thu, 7 Dec 2023 09:13:28 -0800 Subject: [PATCH 1/4] Avoid `_Iter_diff_t` in parameters of `std` function templates `_Iter_diff_t` is `typename iterator_traits::difference_type` in C++17-and-earlier, and `iter_difference_t` in C++20-and-later. We use it for the parameter types of a few function templates that are specified to take the former. This is (1) nicely shorter to spell than the long `iterator_traits` type, and (2) possibly a bit cheaper to compile if we can avoid instantiating `iterator_traits`. I convinced myself the difference probably wasn't observable and we made this change hoping to at least see if it would break anything. Four years later, we now know what will break: subsumption. If I pull e.g. `std::next` into a namespace with a `using` declaration and overload it with constrained function template taking the standard-specified parameters I'll get overload ambiguity on our implementation. The parameter mapping (the transformation from the template parameters to the function parameter types) differs in the two overloads so the constrained overload isn't clearly more specialized. This PR changes all function template parameters `_Iter_diff_t` to `typename iterator_traits::difference_type`. I still believe that the difference isn't observable in return types, so I've left them alone. There are also some non-local clang-format changes in `` that I won't try to explain. --- stl/inc/algorithm | 12 ++++++++---- stl/inc/xutility | 13 ++++++++----- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/stl/inc/algorithm b/stl/inc/algorithm index 77666c2ba4a..95900a911bc 100644 --- a/stl/inc/algorithm +++ b/stl/inc/algorithm @@ -5412,7 +5412,8 @@ void random_shuffle(_RanIt _First, _RanIt _Last) { // shuffle [_First, _Last) us #if _HAS_CXX20 _EXPORT_STD template -constexpr _FwdIt shift_left(_FwdIt _First, const _FwdIt _Last, _Iter_diff_t<_FwdIt> _Pos_to_shift) { +constexpr _FwdIt shift_left( + _FwdIt _First, const _FwdIt _Last, typename iterator_traits<_FwdIt>::difference_type _Pos_to_shift) { // shift [_First, _Last) left by _Pos_to_shift // positions; returns the end of the resulting range _STL_ASSERT(_Pos_to_shift >= 0, "shift count must be non-negative (N4950 [alg.shift]/1)"); @@ -5446,7 +5447,8 @@ constexpr _FwdIt shift_left(_FwdIt _First, const _FwdIt _Last, _Iter_diff_t<_Fwd } _EXPORT_STD template = 0> -_FwdIt shift_left(_ExPo&&, _FwdIt _First, _FwdIt _Last, _Iter_diff_t<_FwdIt> _Pos_to_shift) noexcept /* terminates */ { +_FwdIt shift_left(_ExPo&&, _FwdIt _First, _FwdIt _Last, + typename iterator_traits<_FwdIt>::difference_type _Pos_to_shift) noexcept /* terminates */ { // shift [_First, _Last) left by _Pos_to_shift positions // not parallelized as benchmarks show it isn't worth it _REQUIRE_CPP17_MUTABLE_ITERATOR(_FwdIt); @@ -5454,7 +5456,8 @@ _FwdIt shift_left(_ExPo&&, _FwdIt _First, _FwdIt _Last, _Iter_diff_t<_FwdIt> _Po } _EXPORT_STD template -constexpr _FwdIt shift_right(_FwdIt _First, const _FwdIt _Last, _Iter_diff_t<_FwdIt> _Pos_to_shift) { +constexpr _FwdIt shift_right( + _FwdIt _First, const _FwdIt _Last, typename iterator_traits<_FwdIt>::difference_type _Pos_to_shift) { // shift [_First, _Last) right by _Pos_to_shift // positions; returns the beginning of the resulting range _STL_ASSERT(_Pos_to_shift >= 0, "shift count must be non-negative (N4950 [alg.shift]/5)"); @@ -5528,7 +5531,8 @@ constexpr _FwdIt shift_right(_FwdIt _First, const _FwdIt _Last, _Iter_diff_t<_Fw } _EXPORT_STD template = 0> -_FwdIt shift_right(_ExPo&&, _FwdIt _First, _FwdIt _Last, _Iter_diff_t<_FwdIt> _Pos_to_shift) noexcept /* terminates */ { +_FwdIt shift_right(_ExPo&&, _FwdIt _First, _FwdIt _Last, + typename iterator_traits<_FwdIt>::difference_type _Pos_to_shift) noexcept /* terminates */ { // shift [_First, _Last) right by _Pos_to_shift positions // not parallelized as benchmarks show it isn't worth it _REQUIRE_CPP17_MUTABLE_ITERATOR(_FwdIt); diff --git a/stl/inc/xutility b/stl/inc/xutility index e97140ac360..df242274bf3 100644 --- a/stl/inc/xutility +++ b/stl/inc/xutility @@ -646,7 +646,7 @@ namespace ranges { public: template requires (_Choice<_Ty>._Strategy != _St::_None) - _NODISCARD constexpr decltype(auto) operator()(_Ty&& _Val) const noexcept(_Choice<_Ty>._No_throw) { + _NODISCARD constexpr decltype(auto) operator()(_Ty && _Val) const noexcept(_Choice<_Ty>._No_throw) { constexpr _St _Strat = _Choice<_Ty>._Strategy; if constexpr (_Strat == _St::_Custom) { @@ -1464,7 +1464,8 @@ constexpr _InIt _Next_iter(_InIt _First) { // increment iterator } _EXPORT_STD template -_NODISCARD _CONSTEXPR17 _InIt next(_InIt _First, _Iter_diff_t<_InIt> _Off = 1) { // increment iterator +_NODISCARD _CONSTEXPR17 _InIt next( + _InIt _First, typename iterator_traits<_InIt>::difference_type _Off = 1) { // increment iterator static_assert(_Is_ranges_input_iter_v<_InIt>, "next requires input iterator"); _STD advance(_First, _Off); @@ -1477,7 +1478,8 @@ constexpr _BidIt _Prev_iter(_BidIt _First) { // decrement iterator } _EXPORT_STD template -_NODISCARD _CONSTEXPR17 _BidIt prev(_BidIt _First, _Iter_diff_t<_BidIt> _Off = 1) { // decrement iterator +_NODISCARD _CONSTEXPR17 _BidIt prev( + _BidIt _First, typename iterator_traits<_BidIt>::difference_type _Off = 1) { // decrement iterator static_assert(_Is_ranges_bidi_iter_v<_BidIt>, "prev requires bidirectional iterator"); _STD advance(_First, -_Off); @@ -1625,7 +1627,8 @@ public: template _BidIt2> friend constexpr void iter_swap(const reverse_iterator& _Left, const reverse_iterator<_BidIt2>& _Right) noexcept( - is_nothrow_copy_constructible_v<_BidIt>&& is_nothrow_copy_constructible_v<_BidIt2>&& noexcept( + is_nothrow_copy_constructible_v<_BidIt> + && is_nothrow_copy_constructible_v<_BidIt2>&& noexcept( _RANGES iter_swap(--_STD declval<_BidIt&>(), --_STD declval<_BidIt2&>()))) { auto _LTmp = _Left.current; auto _RTmp = _Right.base(); @@ -5137,7 +5140,7 @@ _INLINE_VAR constexpr bool _Is_pointer_address_comparable = #pragma warning(push) #pragma warning(disable : 4806) // no value of type 'bool' promoted to type 'char' can equal the given constant template && is_integral_v<_Elem2>> + bool = sizeof(_Elem1) == sizeof(_Elem2) && is_integral_v<_Elem1> && is_integral_v<_Elem2>> _INLINE_VAR constexpr bool _Can_memcmp_elements = is_same_v<_Elem1, bool> || is_same_v<_Elem2, bool> || static_cast<_Elem1>(-1) == static_cast<_Elem2>(-1); #pragma warning(pop) From 008e5208e69865a2e9da231717d28fc92343361b Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Thu, 7 Dec 2023 09:43:45 -0800 Subject: [PATCH 2/4] Fix bad formatting --- stl/inc/xutility | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/stl/inc/xutility b/stl/inc/xutility index df242274bf3..48d77a3b4b7 100644 --- a/stl/inc/xutility +++ b/stl/inc/xutility @@ -646,7 +646,7 @@ namespace ranges { public: template requires (_Choice<_Ty>._Strategy != _St::_None) - _NODISCARD constexpr decltype(auto) operator()(_Ty && _Val) const noexcept(_Choice<_Ty>._No_throw) { + _NODISCARD constexpr decltype(auto) operator()(_Ty&& _Val) const noexcept(_Choice<_Ty>._No_throw) { constexpr _St _Strat = _Choice<_Ty>._Strategy; if constexpr (_Strat == _St::_Custom) { @@ -1627,8 +1627,7 @@ public: template _BidIt2> friend constexpr void iter_swap(const reverse_iterator& _Left, const reverse_iterator<_BidIt2>& _Right) noexcept( - is_nothrow_copy_constructible_v<_BidIt> - && is_nothrow_copy_constructible_v<_BidIt2>&& noexcept( + is_nothrow_copy_constructible_v<_BidIt>&& is_nothrow_copy_constructible_v<_BidIt2>&& noexcept( _RANGES iter_swap(--_STD declval<_BidIt&>(), --_STD declval<_BidIt2&>()))) { auto _LTmp = _Left.current; auto _RTmp = _Right.base(); @@ -5140,7 +5139,7 @@ _INLINE_VAR constexpr bool _Is_pointer_address_comparable = #pragma warning(push) #pragma warning(disable : 4806) // no value of type 'bool' promoted to type 'char' can equal the given constant template && is_integral_v<_Elem2>> + bool = sizeof(_Elem1) == sizeof(_Elem2) && is_integral_v<_Elem1>&& is_integral_v<_Elem2>> _INLINE_VAR constexpr bool _Can_memcmp_elements = is_same_v<_Elem1, bool> || is_same_v<_Elem2, bool> || static_cast<_Elem1>(-1) == static_cast<_Elem2>(-1); #pragma warning(pop) From cdd8d4381799ac0e2e65ecb37f10ca2b09f7be5d Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Wed, 13 Dec 2023 15:58:06 -0800 Subject: [PATCH 3/4] Test coverage --- tests/std/test.lst | 1 + .../std/tests/VSO_1925201_iter_traits/env.lst | 4 ++ .../test.compile.pass.cpp | 49 +++++++++++++++++++ 3 files changed, 54 insertions(+) create mode 100644 tests/std/tests/VSO_1925201_iter_traits/env.lst create mode 100644 tests/std/tests/VSO_1925201_iter_traits/test.compile.pass.cpp diff --git a/tests/std/test.lst b/tests/std/test.lst index 825f2055c25..d0c3e68a9b4 100644 --- a/tests/std/test.lst +++ b/tests/std/test.lst @@ -670,6 +670,7 @@ tests\VSO_0000000_instantiate_containers tests\VSO_0000000_instantiate_cvt tests\VSO_0000000_instantiate_iterators_misc tests\VSO_0000000_instantiate_type_traits +tests\VSO_1925201_iter_traits tests\VSO_0000000_list_iterator_debugging tests\VSO_0000000_list_unique_self_reference tests\VSO_0000000_matching_npos_address diff --git a/tests/std/tests/VSO_1925201_iter_traits/env.lst b/tests/std/tests/VSO_1925201_iter_traits/env.lst new file mode 100644 index 00000000000..d6d824b5879 --- /dev/null +++ b/tests/std/tests/VSO_1925201_iter_traits/env.lst @@ -0,0 +1,4 @@ +# Copyright (c) Microsoft Corporation. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +RUNALL_INCLUDE ..\concepts_20_matrix.lst diff --git a/tests/std/tests/VSO_1925201_iter_traits/test.compile.pass.cpp b/tests/std/tests/VSO_1925201_iter_traits/test.compile.pass.cpp new file mode 100644 index 00000000000..279e1fccd96 --- /dev/null +++ b/tests/std/tests/VSO_1925201_iter_traits/test.compile.pass.cpp @@ -0,0 +1,49 @@ +// Copyright (c) Microsoft Corporation. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#include +#include +#include +#include + +// Defend against regression of DevCom-10532126, in which several function templates used +// `_Iter_diff_t` as a parameter type instead of the specified +// `typename iterator_traits::difference_type. The two are equivalent in C++17, but in C++20 +// _Iter_diff_t becomes iter_difference_t. We thought the difference was not observable, +// but it interferes with concept overloading. + +using std::iter_value_t, std::iterator_traits, std::same_as; +using std::next, std::prev, std::shift_left, std::shift_right; + +struct meow {}; + +constexpr meow* nil = nullptr; + +template +concept Meowerator = same_as, meow>; + +template +void next(I, typename iterator_traits::difference_type = 1) {} + +template +void prev(I, typename iterator_traits::difference_type = 1) {} + +template +void shift_left(I, I, typename iterator_traits::difference_type) {} +template +void shift_right(I, I, typename iterator_traits::difference_type) {} +// Note that we intentionally do not test the ExecutionPolicy overloads of shift_meow. They are +// constrained via an unspecified mechanism to "not participate in overload resolution unless +// is_execution_policy_v is true", effectively making concept overloading +// impossible (or at least non-portable). + +static_assert(same_as); +static_assert(same_as); + +static_assert(same_as); +static_assert(same_as); + +#ifndef __clang__ // TRANSITION, LLVM-75404 +static_assert(same_as); +static_assert(same_as); +#endif // TRANSITION, LLVM-75404 From 4264663d5a8154d09e28309589cdd358303a6d55 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 14 Dec 2023 05:40:28 -0800 Subject: [PATCH 4/4] Code review feedback. --- tests/std/test.lst | 2 +- tests/std/tests/VSO_1925201_iter_traits/test.compile.pass.cpp | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/std/test.lst b/tests/std/test.lst index d0c3e68a9b4..ee4fa316ece 100644 --- a/tests/std/test.lst +++ b/tests/std/test.lst @@ -670,7 +670,6 @@ tests\VSO_0000000_instantiate_containers tests\VSO_0000000_instantiate_cvt tests\VSO_0000000_instantiate_iterators_misc tests\VSO_0000000_instantiate_type_traits -tests\VSO_1925201_iter_traits tests\VSO_0000000_list_iterator_debugging tests\VSO_0000000_list_unique_self_reference tests\VSO_0000000_matching_npos_address @@ -733,3 +732,4 @@ tests\VSO_0849827_multicontainer_emplace_hint_position tests\VSO_0938757_attribute_order tests\VSO_0961751_hash_range_erase tests\VSO_0971246_legacy_await_headers +tests\VSO_1925201_iter_traits diff --git a/tests/std/tests/VSO_1925201_iter_traits/test.compile.pass.cpp b/tests/std/tests/VSO_1925201_iter_traits/test.compile.pass.cpp index 279e1fccd96..35d0eeff053 100644 --- a/tests/std/tests/VSO_1925201_iter_traits/test.compile.pass.cpp +++ b/tests/std/tests/VSO_1925201_iter_traits/test.compile.pass.cpp @@ -3,12 +3,11 @@ #include #include -#include #include // Defend against regression of DevCom-10532126, in which several function templates used // `_Iter_diff_t` as a parameter type instead of the specified -// `typename iterator_traits::difference_type. The two are equivalent in C++17, but in C++20 +// `typename iterator_traits::difference_type`. The two are equivalent in C++17, but in C++20 // _Iter_diff_t becomes iter_difference_t. We thought the difference was not observable, // but it interferes with concept overloading.