From 5191e21a950565fe24cff214cc3eff5260eda14b Mon Sep 17 00:00:00 2001 From: Michael Schellenberger Costa Date: Fri, 29 Jan 2021 20:27:50 +0100 Subject: [PATCH 01/10] Noralize next_arg_id --- stl/inc/format | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/stl/inc/format b/stl/inc/format index 16466b6980d..86d734bc142 100644 --- a/stl/inc/format +++ b/stl/inc/format @@ -343,13 +343,12 @@ public: // _Next_arg_id > 0 means automatic // _Next_arg_id == -1 means manual constexpr size_t next_arg_id() { - if (_Next_arg_id >= 0) { - return _Next_arg_id++; + if (_Next_arg_id == -1) { + throw format_error("Can not switch from manual to automatic indexing"); } - throw format_error("Can not switch from manual to automatic indexing"); + return _Next_arg_id++; } - constexpr void check_arg_id(size_t _Id) { - (void) _Id; + constexpr void check_arg_id(size_t) { if (_Next_arg_id > 0) { throw format_error("Can not switch from automatic to manual indexing"); } From 6a4ff26e7b21e9fb105c073e24f64b3b384474b1 Mon Sep 17 00:00:00 2001 From: Michael Schellenberger Costa Date: Fri, 29 Jan 2021 20:35:37 +0100 Subject: [PATCH 02/10] Add precondition check to basic_format_parse_context::advance_to and debug optimization --- stl/inc/format | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/stl/inc/format b/stl/inc/format index 86d734bc142..fbb34356ed5 100644 --- a/stl/inc/format +++ b/stl/inc/format @@ -333,8 +333,9 @@ public: _NODISCARD constexpr const_iterator end() const noexcept { return _Format_string.end(); } - constexpr void advance_to(const_iterator _It) { - _Format_string.remove_prefix(_It - begin()); + constexpr void advance_to(const const_iterator _It) { + _Adl_verify_range(_It, _Format_string.end()); + _Format_string.remove_prefix(_It - _Format_string.begin()); } // While the standard presents an exposition only enum value for From be1afc032633a78911e9d93b2ad4b2fc461d041f Mon Sep 17 00:00:00 2001 From: Michael Schellenberger Costa Date: Fri, 29 Jan 2021 20:41:31 +0100 Subject: [PATCH 03/10] ensure basic_format_parse_context::check_arg_id is not a constant expression --- stl/inc/format | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/stl/inc/format b/stl/inc/format index fbb34356ed5..be583cac137 100644 --- a/stl/inc/format +++ b/stl/inc/format @@ -349,7 +349,15 @@ public: } return _Next_arg_id++; } - constexpr void check_arg_id(size_t) { + constexpr void check_arg_id(const size_t _Id) { + if (_STD is_constant_evaluated()) { + if (_Id >= _Num_args) { + _TRY_BEGIN + throw 42; + _CATCH(42) + _CATCH_END + } + } if (_Next_arg_id > 0) { throw format_error("Can not switch from automatic to manual indexing"); } From ae417d7f2687a697f0f14c8e83beba1a69875efc Mon Sep 17 00:00:00 2001 From: Michael Schellenberger Costa Date: Fri, 29 Jan 2021 21:51:08 +0100 Subject: [PATCH 04/10] Add tests for basic_format_parse_context --- stl/inc/format | 9 +- tests/std/test.lst | 1 + .../P0645R10_text_formatting_death/env.lst | 4 + .../P0645R10_text_formatting_death/test.cpp | 30 ++++++ .../test.cpp | 91 ++++++++++++++++++- 5 files changed, 130 insertions(+), 5 deletions(-) create mode 100644 tests/std/tests/P0645R10_text_formatting_death/env.lst create mode 100644 tests/std/tests/P0645R10_text_formatting_death/test.cpp diff --git a/stl/inc/format b/stl/inc/format index be583cac137..f1faadc5e64 100644 --- a/stl/inc/format +++ b/stl/inc/format @@ -314,6 +314,8 @@ public: using iterator = const_iterator; private: + using _Size_type = typename basic_string_view<_CharT>::size_type; + basic_string_view<_CharT> _Format_string; size_t _Num_args; // The standard says this is size_t, however we use ptrdiff_t to save some space @@ -335,7 +337,8 @@ public: } constexpr void advance_to(const const_iterator _It) { _Adl_verify_range(_It, _Format_string.end()); - _Format_string.remove_prefix(_It - _Format_string.begin()); + const auto _Diff = static_cast<_Size_type>(_It - _Format_string.begin()); + _Format_string.remove_prefix(_Diff); } // While the standard presents an exposition only enum value for @@ -347,14 +350,14 @@ public: if (_Next_arg_id == -1) { throw format_error("Can not switch from manual to automatic indexing"); } - return _Next_arg_id++; + return static_cast(_Next_arg_id++); } constexpr void check_arg_id(const size_t _Id) { if (_STD is_constant_evaluated()) { if (_Id >= _Num_args) { _TRY_BEGIN throw 42; - _CATCH(42) + _CATCH_ALL _CATCH_END } } diff --git a/tests/std/test.lst b/tests/std/test.lst index de1a48a5e19..7d5f3a8123d 100644 --- a/tests/std/test.lst +++ b/tests/std/test.lst @@ -243,6 +243,7 @@ tests\P0595R2_is_constant_evaluated tests\P0607R0_inline_variables tests\P0616R0_using_move_in_numeric tests\P0631R8_numbers_math_constants +tests\P0645R10_text_formatting_death tests\P0645R10_text_formatting_parse_contexts tests\P0645R10_text_formatting_parsing tests\P0660R10_jthread_and_cv_any diff --git a/tests/std/tests/P0645R10_text_formatting_death/env.lst b/tests/std/tests/P0645R10_text_formatting_death/env.lst new file mode 100644 index 00000000000..f3ccc8613c6 --- /dev/null +++ b/tests/std/tests/P0645R10_text_formatting_death/env.lst @@ -0,0 +1,4 @@ +# Copyright (c) Microsoft Corporation. +# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +RUNALL_INCLUDE ..\concepts_matrix.lst diff --git a/tests/std/tests/P0645R10_text_formatting_death/test.cpp b/tests/std/tests/P0645R10_text_formatting_death/test.cpp new file mode 100644 index 00000000000..86e74da6037 --- /dev/null +++ b/tests/std/tests/P0645R10_text_formatting_death/test.cpp @@ -0,0 +1,30 @@ +// Copyright (c) Microsoft Corporation. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#define _CONTAINER_DEBUG_LEVEL 1 + +#include +#include +#include + +#include +using namespace std; + +void test_case_advance_no_range() { + const auto format_string = "First {} and second {} and third {}"sv; + const auto other_format_string = "something"sv; + basic_format_parse_context context{format_string}; + context.advance_to(other_format_string.begin()); +} + +int main(int argc, char* argv[]) { + std_testing::death_test_executive exec; + +#if _ITERATOR_DEBUG_LEVEL != 0 + exec.add_death_tests({ + test_case_advance_no_range, + }); +#endif // _ITERATOR_DEBUG_LEVEL != 0 + + return exec.run(argc, argv); +} diff --git a/tests/std/tests/P0645R10_text_formatting_parse_contexts/test.cpp b/tests/std/tests/P0645R10_text_formatting_parse_contexts/test.cpp index 7a3f5c0dad2..6d49094c194 100644 --- a/tests/std/tests/P0645R10_text_formatting_parse_contexts/test.cpp +++ b/tests/std/tests/P0645R10_text_formatting_parse_contexts/test.cpp @@ -2,11 +2,98 @@ // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception #include +#include #include #include -// TODO: fill in tests +using namespace std; +using namespace literals; + +template +constexpr auto get_input() { + if constexpr (same_as) { + return "First {} and second {} and third {}"sv; + } else { + return L"First {} and second {} and third {}"sv; + } +} + +template +bool ensure_not_constant_expression() { + basic_format_parse_context context{get_input()}; + context.check_arg_id(1); + if (is_constant_evaluated()) { + return false; + } else { + return true; + } +} + +const bool check_arg_id_not_constexpr_char = ensure_not_constant_expression(); +const bool check_arg_id_not_constexpr_wchar = ensure_not_constant_expression(); + +template +constexpr bool test_basic_format_parse_context() { + static_assert(!is_copy_constructible_v>); + static_assert(!is_copy_assignable_v>); + + const auto format_string = get_input(); + { // iterator interface + basic_format_parse_context context{format_string}; + const same_as::const_iterator> auto b = context.begin(); + assert(b == format_string.begin()); + static_assert(noexcept(context.begin())); + + const same_as::const_iterator> auto e = context.end(); + assert(e == format_string.end()); + static_assert(noexcept(context.end())); + + const auto new_position = b + 5; + context.advance_to(new_position); + assert(to_address(context.begin()) == to_address(new_position)); + assert(to_address(context.end()) == to_address(e)); + } + + { // arg_id + basic_format_parse_context context{format_string}; + const same_as auto first_arg_id = context.next_arg_id(); + assert(first_arg_id == 0); + + const same_as auto second_arg_id = context.next_arg_id(); + assert(second_arg_id == 1); + + if (!is_constant_evaluated()) { + try { + context.check_arg_id(0); + } catch (format_error e) { + assert(e.what() == "Can not switch from automatic to manual indexing"sv); + } + } + } + + { // check_arg_id + basic_format_parse_context context{format_string, 3}; + context.check_arg_id(1); + context.check_arg_id(1); + + if (!is_constant_evaluated()) { + try { + context.next_arg_id(); + } catch (format_error e) { + assert(e.what() == "Can not switch from manual to automatic indexing"sv); + } + } + } + + return true; +} int main() { - return 0; + test_basic_format_parse_context(); + test_basic_format_parse_context(); + static_assert(test_basic_format_parse_context()); + static_assert(test_basic_format_parse_context()); + + assert(check_arg_id_not_constexpr_char); + assert(check_arg_id_not_constexpr_wchar); } From cbea0a50bdf1c15ffcd67d9e82f64cedac897289 Mon Sep 17 00:00:00 2001 From: Michael Schellenberger Costa Date: Fri, 29 Jan 2021 22:09:02 +0100 Subject: [PATCH 05/10] Fix constant expression check --- .../test.cpp | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/tests/std/tests/P0645R10_text_formatting_parse_contexts/test.cpp b/tests/std/tests/P0645R10_text_formatting_parse_contexts/test.cpp index 6d49094c194..1a8b59c9fa9 100644 --- a/tests/std/tests/P0645R10_text_formatting_parse_contexts/test.cpp +++ b/tests/std/tests/P0645R10_text_formatting_parse_contexts/test.cpp @@ -19,18 +19,24 @@ constexpr auto get_input() { } template -bool ensure_not_constant_expression() { - basic_format_parse_context context{get_input()}; - context.check_arg_id(1); - if (is_constant_evaluated()) { - return false; +constexpr bool ensure_is_constant_expression(const bool should_be_constant_expression) { + basic_format_parse_context context{get_input(), 1}; + if (should_be_constant_expression) { + context.check_arg_id(0); } else { + context.check_arg_id(1); + } + if (is_constant_evaluated()) { return true; + } else { + return false; } } -const bool check_arg_id_not_constexpr_char = ensure_not_constant_expression(); -const bool check_arg_id_not_constexpr_wchar = ensure_not_constant_expression(); +const bool check_arg_id_is_constexpr_char = ensure_is_constant_expression(true); +const bool check_arg_id_not_constexpr_char = not ensure_is_constant_expression(false); +const bool check_arg_id_is_constexpr_wchar = ensure_is_constant_expression(true); +const bool check_arg_id_not_constexpr_wchar = not ensure_is_constant_expression(false); template constexpr bool test_basic_format_parse_context() { @@ -94,6 +100,8 @@ int main() { static_assert(test_basic_format_parse_context()); static_assert(test_basic_format_parse_context()); + assert(check_arg_id_is_constexpr_char); assert(check_arg_id_not_constexpr_char); + assert(check_arg_id_is_constexpr_wchar); assert(check_arg_id_not_constexpr_wchar); } From ba63570faa80b5b09675baba1b8da1fffe88b0fc Mon Sep 17 00:00:00 2001 From: Michael Schellenberger Costa Date: Fri, 29 Jan 2021 22:31:29 +0100 Subject: [PATCH 06/10] Not not --- .../tests/P0645R10_text_formatting_parse_contexts/test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/std/tests/P0645R10_text_formatting_parse_contexts/test.cpp b/tests/std/tests/P0645R10_text_formatting_parse_contexts/test.cpp index 1a8b59c9fa9..08c0e777a63 100644 --- a/tests/std/tests/P0645R10_text_formatting_parse_contexts/test.cpp +++ b/tests/std/tests/P0645R10_text_formatting_parse_contexts/test.cpp @@ -34,9 +34,9 @@ constexpr bool ensure_is_constant_expression(const bool should_be_constant_expre } const bool check_arg_id_is_constexpr_char = ensure_is_constant_expression(true); -const bool check_arg_id_not_constexpr_char = not ensure_is_constant_expression(false); +const bool check_arg_id_not_constexpr_char = !ensure_is_constant_expression(false); const bool check_arg_id_is_constexpr_wchar = ensure_is_constant_expression(true); -const bool check_arg_id_not_constexpr_wchar = not ensure_is_constant_expression(false); +const bool check_arg_id_not_constexpr_wchar = !ensure_is_constant_expression(false); template constexpr bool test_basic_format_parse_context() { From 816fe40eab67d5556995c093f3e950cd121c1b7c Mon Sep 17 00:00:00 2001 From: Michael Schellenberger Costa Date: Fri, 29 Jan 2021 22:32:58 +0100 Subject: [PATCH 07/10] windsdk is bad --- tests/std/tests/P0645R10_text_formatting_death/env.lst | 2 +- tests/std/tests/P0645R10_text_formatting_parse_contexts/env.lst | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/std/tests/P0645R10_text_formatting_death/env.lst b/tests/std/tests/P0645R10_text_formatting_death/env.lst index f3ccc8613c6..22f1f0230a4 100644 --- a/tests/std/tests/P0645R10_text_formatting_death/env.lst +++ b/tests/std/tests/P0645R10_text_formatting_death/env.lst @@ -1,4 +1,4 @@ # Copyright (c) Microsoft Corporation. # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -RUNALL_INCLUDE ..\concepts_matrix.lst +RUNALL_INCLUDE ..\strict_winsdk_concepts_matrix.lst diff --git a/tests/std/tests/P0645R10_text_formatting_parse_contexts/env.lst b/tests/std/tests/P0645R10_text_formatting_parse_contexts/env.lst index f3ccc8613c6..22f1f0230a4 100644 --- a/tests/std/tests/P0645R10_text_formatting_parse_contexts/env.lst +++ b/tests/std/tests/P0645R10_text_formatting_parse_contexts/env.lst @@ -1,4 +1,4 @@ # Copyright (c) Microsoft Corporation. # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -RUNALL_INCLUDE ..\concepts_matrix.lst +RUNALL_INCLUDE ..\strict_winsdk_concepts_matrix.lst From 5485658cfe41d1e7d55463f5cb011e52574c4c7f Mon Sep 17 00:00:00 2001 From: Michael Schellenberger Costa Date: Sat, 30 Jan 2021 12:45:12 +0100 Subject: [PATCH 08/10] Address review comments --- stl/inc/format | 29 ++++++++++--------- .../test.cpp | 20 +++++++------ 2 files changed, 27 insertions(+), 22 deletions(-) diff --git a/stl/inc/format b/stl/inc/format index f1faadc5e64..1c89620f3ad 100644 --- a/stl/inc/format +++ b/stl/inc/format @@ -305,7 +305,6 @@ constexpr const _CharT* _Parse_format_specs(const _CharT* _Begin, const _CharT* template struct formatter; -// TODO: test coverage template class basic_format_parse_context { public: @@ -313,17 +312,6 @@ public: using const_iterator = typename basic_string_view<_CharT>::const_iterator; using iterator = const_iterator; -private: - using _Size_type = typename basic_string_view<_CharT>::size_type; - - basic_string_view<_CharT> _Format_string; - size_t _Num_args; - // The standard says this is size_t, however we use ptrdiff_t to save some space - // by not having to store the indexing mode. Below is a more detailed explanation - // of how this works. - ptrdiff_t _Next_arg_id = 0; - -public: constexpr explicit basic_format_parse_context(basic_string_view<_CharT> _Fmt, size_t _Num_args_ = 0) noexcept : _Format_string(_Fmt), _Num_args(_Num_args_) {} basic_format_parse_context(const basic_format_parse_context&) = delete; @@ -337,7 +325,7 @@ public: } constexpr void advance_to(const const_iterator _It) { _Adl_verify_range(_It, _Format_string.end()); - const auto _Diff = static_cast<_Size_type>(_It - _Format_string.begin()); + const auto _Diff = static_cast(_It._Unwrapped() - _Format_string._Unchecked_begin()); _Format_string.remove_prefix(_Diff); } @@ -350,22 +338,37 @@ public: if (_Next_arg_id == -1) { throw format_error("Can not switch from manual to automatic indexing"); } + return static_cast(_Next_arg_id++); } + constexpr void check_arg_id(const size_t _Id) { if (_STD is_constant_evaluated()) { if (_Id >= _Num_args) { +#ifdef __clang__ // TRANSITION clang-12 _TRY_BEGIN throw 42; _CATCH_ALL _CATCH_END +#else // ^^^ workaround ^^^ / vvv no workaround vvv + [[maybe_unused]] const auto _Reinterpret_to_make_not_constexpr = reinterpret_cast(&_Num_args); +#endif // !__clang__ } } + if (_Next_arg_id > 0) { throw format_error("Can not switch from automatic to manual indexing"); } _Next_arg_id = -1; } + +private: + basic_string_view<_CharT> _Format_string; + size_t _Num_args; + // The standard says this is size_t, however we use ptrdiff_t to save some space + // by not having to store the indexing mode. Below is a more detailed explanation + // of how this works. + ptrdiff_t _Next_arg_id = 0; }; // TODO: test coverage diff --git a/tests/std/tests/P0645R10_text_formatting_parse_contexts/test.cpp b/tests/std/tests/P0645R10_text_formatting_parse_contexts/test.cpp index 08c0e777a63..606f08b51f4 100644 --- a/tests/std/tests/P0645R10_text_formatting_parse_contexts/test.cpp +++ b/tests/std/tests/P0645R10_text_formatting_parse_contexts/test.cpp @@ -3,11 +3,13 @@ #include #include +#include #include +#include #include +#include using namespace std; -using namespace literals; template constexpr auto get_input() { @@ -26,11 +28,8 @@ constexpr bool ensure_is_constant_expression(const bool should_be_constant_expre } else { context.check_arg_id(1); } - if (is_constant_evaluated()) { - return true; - } else { - return false; - } + + return is_constant_evaluated(); } const bool check_arg_id_is_constexpr_char = ensure_is_constant_expression(true); @@ -54,7 +53,7 @@ constexpr bool test_basic_format_parse_context() { assert(e == format_string.end()); static_assert(noexcept(context.end())); - const auto new_position = b + 5; + const auto new_position = format_string.begin() + 5; context.advance_to(new_position); assert(to_address(context.begin()) == to_address(new_position)); assert(to_address(context.end()) == to_address(e)); @@ -71,7 +70,7 @@ constexpr bool test_basic_format_parse_context() { if (!is_constant_evaluated()) { try { context.check_arg_id(0); - } catch (format_error e) { + } catch (const format_error& e) { assert(e.what() == "Can not switch from automatic to manual indexing"sv); } } @@ -79,13 +78,16 @@ constexpr bool test_basic_format_parse_context() { { // check_arg_id basic_format_parse_context context{format_string, 3}; + context.check_arg_id(0); context.check_arg_id(1); + context.check_arg_id(2); // intentional duplicates to check whether this is ok to call multiple times context.check_arg_id(1); + context.check_arg_id(0); if (!is_constant_evaluated()) { try { context.next_arg_id(); - } catch (format_error e) { + } catch (const format_error& e) { assert(e.what() == "Can not switch from manual to automatic indexing"sv); } } From e938cfaad905e2a244f958b228ab33cc50b7f3c6 Mon Sep 17 00:00:00 2001 From: Michael Schellenberger Costa Date: Sun, 31 Jan 2021 12:06:00 +0100 Subject: [PATCH 09/10] Use a non constexpr function call with a better name Co-authored-by: S. B. Tam --- stl/inc/format | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/stl/inc/format b/stl/inc/format index 1c89620f3ad..22b8daefc60 100644 --- a/stl/inc/format +++ b/stl/inc/format @@ -305,6 +305,8 @@ constexpr const _CharT* _Parse_format_specs(const _CharT* _Begin, const _CharT* template struct formatter; +inline void _You_see_this_error_because_arg_id_is_out_of_range() noexcept {} + template class basic_format_parse_context { public: @@ -345,14 +347,7 @@ public: constexpr void check_arg_id(const size_t _Id) { if (_STD is_constant_evaluated()) { if (_Id >= _Num_args) { -#ifdef __clang__ // TRANSITION clang-12 - _TRY_BEGIN - throw 42; - _CATCH_ALL - _CATCH_END -#else // ^^^ workaround ^^^ / vvv no workaround vvv - [[maybe_unused]] const auto _Reinterpret_to_make_not_constexpr = reinterpret_cast(&_Num_args); -#endif // !__clang__ + _You_see_this_error_because_arg_id_is_out_of_range(); } } From 2ed5591350e25fe32ffe5a5b92bc6b441c34d6e0 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Thu, 4 Feb 2021 19:53:06 -0800 Subject: [PATCH 10/10] Reverse the polarity --- stl/inc/format | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/format b/stl/inc/format index 22b8daefc60..f968858c563 100644 --- a/stl/inc/format +++ b/stl/inc/format @@ -361,7 +361,7 @@ private: basic_string_view<_CharT> _Format_string; size_t _Num_args; // The standard says this is size_t, however we use ptrdiff_t to save some space - // by not having to store the indexing mode. Below is a more detailed explanation + // by not having to store the indexing mode. Above is a more detailed explanation // of how this works. ptrdiff_t _Next_arg_id = 0; };