`<xstring>`: Strengthen exception specification for `sv.compare("NTBS")` by frederick-vs-ja · Pull Request #3738 · microsoft/STL

frederick-vs-ja · 2023-05-28T01:40:11Z

As discovered in #3735, both

basic_string_view's constructor from a const _Elem* (strengthened) and
the basic_string_view::compare overload for two basic_string_views (mandatory)

are already noexcept, so basic_string_view::compare for a basic_string_view and a const _Elem* should also be noexcept.

The exception specification strengthening is already done for basic_string.

STL/stl/inc/xstring

Lines 4646 to 4649 in a621095

    
           _NODISCARD _CONSTEXPR20 int compare(_In_z_ const _Elem* const _Ptr) const noexcept /* strengthened */ {
 
               // compare [0, size()) with [_Ptr, <null>)
 
               return _Traits_compare<_Traits>(_Mypair._Myval2._Myptr(), _Mypair._Myval2._Mysize, _Ptr, _Traits::length(_Ptr));
 
           }

The message in the static_assert "(this is bad, char_traits)" no longer holds, since the exception specifications of all char_traits::compare in MSVC STL are already strengthened.

Driven-by changes: make basic_string::_Eos, basic_string::_Swap_proxy_and_iterators, and basic_string::_Swap_data noexcept since they are non-throwing and already called by noexcept functions.

achabense · 2023-05-28T04:42:54Z

More candidates:
_Move_assign_from_buffer and _Copy_assign_val_from_small:
https://github.com/microsoft/STL/blob/a62109595b6d89e08172fdf4beb75a2670fe0cc9/stl/inc/xstring#L2908?plain=1
https://github.com/microsoft/STL/blob/a62109595b6d89e08172fdf4beb75a2670fe0cc9/stl/inc/xstring#L3131?plain=1
_Become_small then reserve(void):
https://github.com/microsoft/STL/blob/a62109595b6d89e08172fdf4beb75a2670fe0cc9/stl/inc/xstring#L4796?plain=1
https://github.com/microsoft/STL/blob/a62109595b6d89e08172fdf4beb75a2670fe0cc9/stl/inc/xstring#L4191?plain=1
_Eos:
https://github.com/microsoft/STL/blob/a62109595b6d89e08172fdf4beb75a2670fe0cc9/stl/inc/xstring#L4813?plain=1
_Swap_proxy_and_iterators and _Swap_data:
https://github.com/microsoft/STL/blob/a62109595b6d89e08172fdf4beb75a2670fe0cc9/stl/inc/xstring#L4853?plain=1
https://github.com/microsoft/STL/blob/a62109595b6d89e08172fdf4beb75a2670fe0cc9/stl/inc/xstring#L4261?plain=1

Since they are already called by `noexcept` functions.

frederick-vs-ja · 2023-05-28T06:18:40Z

I think we should do strengthening for _Eos, _Swap_proxy_and_iterators, and _Swap_data since they are already called by noexcept functions.

I'm hesitant about other functions, because

_Move_assign_from_buffer and _Copy_assign_val_from_small are only called from functions that may throw an exception,
reserve(void) is deprecated, and other invocations of _Become_small are only from functions that may throw an exception (like the above).

achabense · 2023-05-28T08:33:54Z

_Move_assign_from_buffer:
It doesn't involve memory allocation or other throwable operations.
The caller sstream::str()&& (here) is throwable due to assignment in another place.

_Copy_assign_val_from_small:
It doesn't involve memory allocation or other throwable operations.
The caller operator=(const basic_string&) (here) is throwable due to assignment in another place.
(off topic) I saw // TRANSITION, VSO-761321; inline into only caller when that's fixed in its code. What is it? Can it be fixed now?

_Become_small:
It's used to deallocate superfluous memory. Compared with _Tidy_deallocate (here) I believe it's safe to add noexcept.
The callers are reserves. They are throwable due to some potential re-allocations. As to reserve(void), it doesn't involve such kind of re-allocation. Though deprecated, a noexcept /*strengthened*/ is very cheap to add so I think it's ok.

(off topic)
basic_string(nullptr_t) is now inserted between basic_string(const _Elem*) and basic_string(const _Elem*, const _Alloc& ). The latter two work as a whole to resolve as basic_string(const _Elem*, const _Alloc& = _Alloc()). For better maintainability, please consider moving basic_string(nullptr_t) elsewhere.

STL/stl/inc/xstring

Lines 2539 to 2553 in a621095

    
               _CONSTEXPR20 basic_string(_In_z_ const _Elem* const _Ptr) : _Mypair(_Zero_then_variadic_args_t{}) {
 
                   _Construct<_Construct_strategy::_From_ptr>(_Ptr, _Convert_size<size_type>(_Traits::length(_Ptr)));
 
               }
 
           #if _HAS_CXX23
 
               basic_string(nullptr_t) = delete;
 
           #endif // _HAS_CXX23
 
           #if _HAS_CXX17
 
               template <class _Alloc2 = _Alloc, enable_if_t<_Is_allocator<_Alloc2>::value, int> = 0>
 
           #endif // _HAS_CXX17
 
               _CONSTEXPR20 basic_string(_In_z_ const _Elem* const _Ptr, const _Alloc& _Al)
 
                   : _Mypair(_One_then_variadic_args_t{}, _Al) {
 
                   _Construct<_Construct_strategy::_From_ptr>(_Ptr, _Convert_size<size_type>(_Traits::length(_Ptr)));
 
               }

StephanTLavavej · 2023-06-14T19:08:44Z

I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

strega-nil-ms

nit

strega-nil-ms · 2023-06-14T20:51:14Z

@StephanTLavavej minor change pushed

StephanTLavavej · 2023-06-14T20:56:34Z

@strega-nil-ms I believe you have flipped the sense of the message; compare the previous message.

(The message is admittedly confusing about whether it's stating what is currently observed, or what should be observed.)

This reverts commit 5a05f1a.

StephanTLavavej · 2023-06-14T21:55:39Z

@strega-nil-ms @CaseyCarter I've pushed changes to restore the // strengthened comment, then mechanically replace the entire test to use terse static_assert as it was already doing in several places (yay for C++17). For the test suite, static_assert messages are much less useful than in product code, so I didn't restrict this replacement to just the noexcept stuff, I did the whole file.

StephanTLavavej · 2023-06-15T09:23:21Z

🦾 ⚠️ 🧶

frederick-vs-ja added 2 commits May 28, 2023 09:26

Strengthen exception specification for sv.compare(psz)

206dc6f

Correspondingly modify the test file

5e73592

frederick-vs-ja requested a review from a team as a code owner May 28, 2023 01:40

Missed // strengthened comment

f8c484b

Also _Eos, _Swap_proxy_and_iterators, and _Swap_data

a18c3f4

Since they are already called by `noexcept` functions.

StephanTLavavej added the enhancement Something can be improved label May 28, 2023

StephanTLavavej approved these changes May 28, 2023

View reviewed changes

CaseyCarter self-assigned this Jun 7, 2023

StephanTLavavej self-assigned this Jun 14, 2023

strega-nil-ms approved these changes Jun 14, 2023

View reviewed changes

Comment thread tests/std/tests/P0220R1_string_view/test.cpp Outdated

Update tests/std/tests/P0220R1_string_view/test.cpp

5a05f1a

CaseyCarter approved these changes Jun 14, 2023

View reviewed changes

StephanTLavavej unassigned CaseyCarter Jun 14, 2023

StephanTLavavej added 2 commits June 14, 2023 14:41

Revert "Update tests/std/tests/P0220R1_string_view/test.cpp"

5ec275c

This reverts commit 5a05f1a.

Mechanically replace P0220R1_string_view to use terse static_assert.

a58bd4c

StephanTLavavej approved these changes Jun 14, 2023

View reviewed changes

CaseyCarter approved these changes Jun 14, 2023

View reviewed changes

StephanTLavavej merged commit 1573c78 into microsoft:main Jun 15, 2023

frederick-vs-ja deleted the noexcept-compare-ntbs branch June 15, 2023 09:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`<xstring>`: Strengthen exception specification for `sv.compare("NTBS")`#3738

`<xstring>`: Strengthen exception specification for `sv.compare("NTBS")`#3738
StephanTLavavej merged 7 commits into
microsoft:mainfrom
frederick-vs-ja:noexcept-compare-ntbs

frederick-vs-ja commented May 28, 2023 •

edited

Loading

Uh oh!

achabense commented May 28, 2023 •

edited

Loading

Uh oh!

frederick-vs-ja commented May 28, 2023 •

edited

Loading

Uh oh!

achabense commented May 28, 2023 •

edited

Loading

Uh oh!

StephanTLavavej commented Jun 14, 2023

Uh oh!

strega-nil-ms left a comment

Uh oh!

Uh oh!

strega-nil-ms commented Jun 14, 2023 •

edited

Loading

Uh oh!

StephanTLavavej commented Jun 14, 2023

Uh oh!

StephanTLavavej commented Jun 14, 2023

Uh oh!

StephanTLavavej commented Jun 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	_NODISCARD _CONSTEXPR20 int compare(_In_z_ const _Elem* const _Ptr) const noexcept /* strengthened */ {
	// compare [0, size()) with [_Ptr, <null>)
	return _Traits_compare<_Traits>(_Mypair._Myval2._Myptr(), _Mypair._Myval2._Mysize, _Ptr, _Traits::length(_Ptr));
	}

Conversation

frederick-vs-ja commented May 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achabense commented May 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frederick-vs-ja commented May 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achabense commented May 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StephanTLavavej commented Jun 14, 2023

Uh oh!

strega-nil-ms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

strega-nil-ms commented Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StephanTLavavej commented Jun 14, 2023

Uh oh!

StephanTLavavej commented Jun 14, 2023

Uh oh!

StephanTLavavej commented Jun 15, 2023

🦾 ⚠️ 🧶

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

frederick-vs-ja commented May 28, 2023 •

edited

Loading

achabense commented May 28, 2023 •

edited

Loading

frederick-vs-ja commented May 28, 2023 •

edited

Loading

achabense commented May 28, 2023 •

edited

Loading

strega-nil-ms commented Jun 14, 2023 •

edited

Loading