Implement LWG-3662 string::append/assign(NTBS, pos, n) should not construct a temporary string#6235
Implement LWG-3662 string::append/assign(NTBS, pos, n) should not construct a temporary string#6235YexuanXiao wants to merge 1 commit intomicrosoft:mainfrom
Conversation
…struct a temporary string
There was a problem hiding this comment.
Pull request overview
Implements LWG-3662 by adding basic_string::append/assign(const CharT* ntbs, size_type pos, size_type n) overloads that operate directly on the NTBS (avoiding constructing a temporary basic_string), and adds a regression test to ensure the implementation doesn’t rely on default-constructing the allocator.
Changes:
- Add
basic_string::append(const _Elem*, size_type, size_type)implemented via_Traits::length+_Appendwith clamping. - Add
basic_string::assign(const _Elem*, size_type, size_type)implemented via_Traits::length+_Assignwith clamping. - Add
test_LWG3662()using a non-default-constructible allocator to catch temporary-string construction.
Show a summary per file
| File | Description |
|---|---|
| tests/std/tests/VSO_0174871_string_replace/test.cpp | Adds test_LWG3662() regression coverage for the new NTBS pos/n overloads and out-of-range behavior. |
| stl/inc/xstring | Introduces new append/assign(NTBS, pos, n) overloads intended to avoid temporary basic_string construction. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
stl/inc/xstring:1662
- Same SAL issue as the new
appendoverload:_In_reads_(_Count)is misleading for this NTBS overload because_Traits::length(_Ptr)reads until the null terminator regardless of_Count. Please change this to_In_z_for consistency with other NTBS-taking overloads in this header.
_CONSTEXPR20 basic_string& assign(
_In_reads_(_Count) const _Elem* const _Ptr, const size_type _Off, _CRT_GUARDOVERFLOW const size_type _Count) {
// assign(string_view(_Ptr).substr(_Off, _Count))
const size_type _Length = _Convert_size<size_type>(_Traits::length(_Ptr));
if (_Off > _Length) {
- Files reviewed: 2/2 changed files
- Comments generated: 3
| #endif // _HAS_CXX17 | ||
|
|
||
| _CONSTEXPR20 basic_string& append( | ||
| _In_reads_(_Count) const _Elem* const _Ptr, const size_type _Off, _CRT_GUARDOVERFLOW const size_type _Count) { |
There was a problem hiding this comment.
The SAL annotation _In_reads_(_Count) is inaccurate for this NTBS overload because the implementation calls _Traits::length(_Ptr) (reads until null terminator, potentially beyond _Count) and then appends only a clamped subrange. For consistency with other NTBS-taking overloads in this file (e.g. append(_In_z_ const _Elem*)), this should be annotated as _In_z_ (and similarly for assign).
This issue also appears on line 1658 of the same file.
| _In_reads_(_Count) const _Elem* const _Ptr, const size_type _Off, _CRT_GUARDOVERFLOW const size_type _Count) { | |
| _In_z_ const _Elem* const _Ptr, const size_type _Off, _CRT_GUARDOVERFLOW const size_type _Count) { |
| test_shrink_to_fit(); | ||
| test_LWG3662(); |
There was a problem hiding this comment.
If these new basic_string::append/assign(const char*, pos, n) overloads are gated to C++23 mode (as other C++23 basic_string APIs are), this call (and the test_LWG3662 definition) should be wrapped in #if _HAS_CXX23 so the test suite still compiles in earlier language modes.
| test_shrink_to_fit(); | |
| test_LWG3662(); | |
| test_shrink_to_fit(); | |
| #if _HAS_CXX23 | |
| test_LWG3662(); | |
| #endif // _HAS_CXX23 |
There was a problem hiding this comment.
Nope. These overloads should be added to C++14 mode (the earliest mode we support),
| _CONSTEXPR20 basic_string& append( | ||
| _In_reads_(_Count) const _Elem* const _Ptr, const size_type _Off, _CRT_GUARDOVERFLOW const size_type _Count) { | ||
| // append(string_view(_Ptr).substr(_Off, _Count)) | ||
| const size_type _Length = _Convert_size<size_type>(_Traits::length(_Ptr)); | ||
| if (_Off > _Length) { | ||
| _Scary_val::_Xran(); | ||
| } | ||
| return _Append(_Ptr + _Off, (_STD min) (_Length - _Off, _Count)); | ||
| } |
There was a problem hiding this comment.
These new append(const _Elem*, size_type, size_type) and assign(const _Elem*, size_type, size_type) overloads appear to be C++23 additions (similar to other C++23-only basic_string members like contains/append_range which are guarded by _HAS_CXX23). As written, they’re unconditionally available in older language modes, which changes the overload set and can break source conformance/compatibility. Please wrap these overloads in the appropriate feature-test guard (likely #if _HAS_CXX23).
Fixes #6203.