Efficient Access To basic_stringbuf's Buffer #919

Berrysoft · 2020-06-25T15:13:51Z

Fixes #15

Berrysoft · 2020-06-26T13:46:43Z

There're two questions I cannot decide:

According to the standard, the method basic_stringbuf::str() const should be changed to basic_stringbuf::str() const&. I'm not sure if it is an ABI-breaking change.
basic_stringbuf uses a custom buffer, different from basic_string. I try to not change ABI, but it is somehow difficult and dirty, when it comes to small string optimization, and readonly mode of basic_stringbuf. I'm not sure if the current solution is acceptable:
- In readonly mode, copy the string, as the current implementation doesn't recognize the reserved space in readonly mode.
- Otherwise, move the buffer, or copy the string in small mode.

StephanTLavavej

Thanks! I have some initial comments after reviewing all of the files (although I haven't done a line-by-line comparison of the product code to the Standardese yet).

stl/inc/sstream

stl/inc/xstring

stl/inc/yvals_core.h

tests/std/tests/P0408R7_efficient_access_to_stringbuf_buffer/env.lst

tests/std/tests/P0408R7_efficient_access_to_stringbuf_buffer/test.cpp

stl/inc/sstream

StephanTLavavej

Thanks! I have a number of comments but the issues I found were localized - to save time, I will work on fixing them later today.

StephanTLavavej · 2021-01-13T03:15:57Z

stl/inc/xutility

+#if _HAS_CXX20
+template <class _Ty>
+concept _Allocator = _Is_allocator<_Ty>::value;
+#endif // _HAS_CXX20
+


This looks OK to me. I think the unusual part is that this is actually a concept. Elsewhere, we've used SFINAE to implement constraints in C++20 features, except when concepts are absolutely unavoidable (for example <ranges>; also <span> has a conformant concepts and fallback SFINAE implementation). This is because EDG has certain bugs that prevent us from fully enabling concepts at this time.

Given that the tests are passing, and that we ultimately do want to use concepts in C++20 mode, I think this is OK. If we encounter issues with EDG, replacing this concept with SFINAE should be a targeted change.

Separately, it would be possible to implement this concept "directly" instead of by referring to the SFINAE implementation that I wrote for CTAD; however I am not sure that there are any throughput benefits to doing so, and duplicating that somewhat complicated definition would be undesirable, so the centralized approach seems ideal.

stl/inc/xstring

StephanTLavavej · 2021-01-13T03:41:56Z

stl/inc/xstring

+            _Construct_in_place(_My_data._Bx._Ptr, _Fancy_right);
+            return true;
+        } else {
+            _Traits::copy(_My_data._Bx._Buf, _Right, _Res);


I am unsure whether we should copy _Res characters, or _Size + 1 for just the characters that are actually stored (+ 1 for the null terminator).

I don't think this is a correctness issue, so I don't want to risk damaging the code. No change requested.

StephanTLavavej · 2021-01-13T03:43:59Z

stl/inc/xstring

+        } else {
+            // use _BUF_SIZE + 1 to avoid SSO, if the buffer is assigned back
+            _Result._Ptr = _Al.allocate(_BUF_SIZE + 1);
+            _Traits::copy(_Unfancy(_Result._Ptr), _My_data._Bx._Buf, _BUF_SIZE);


Similarly, I am unsure whether we should copy the entire _BUF_SIZE, or just _My_data._Mysize + 1 for the actually stored characters plus the null terminator.

Ditto no change requested.

stl/inc/sstream

tests/std/tests/P0408R7_efficient_access_to_stringbuf_buffer/test.cpp

For basic_istringstream/basic_ostringstream, this fixes correctness. For basic_stringstream, this improves clarity and debug codegen.

Berrysoft · 2021-01-15T05:55:47Z

I think the review request to @BillyONeal was a mishandling by myself, but I don't know how to cancel it.

StephanTLavavej · 2021-01-15T07:33:40Z

@Berrysoft No problem, it won’t block merging. I plan to get a quick double-check from another maintainer tomorrow, then prepare an internal MSVC-PR port.

StephanTLavavej · 2021-01-16T03:39:54Z

Merged! Thanks again for implementing this feature, which will ship in VS 2019 16.10 Preview 1. 🚀 😺 🎉

Add methods & implementations except move ones.

3c0184a

cbezault self-assigned this Jun 25, 2020

StephanTLavavej added the cxx20 C++20 feature label Jun 26, 2020

Berrysoft added 5 commits June 26, 2020 12:19

[WIP] Add new methods to string.

6524bb0

Complete _Release_to_buffer

7f67b8b

Add to yvals_core.

88d4d99

Add test.

e93c67f

Simplify view type.

b1cdf97

Berrysoft marked this pull request as ready for review June 26, 2020 13:50

Berrysoft requested a review from a team as a code owner June 26, 2020 13:50

Berrysoft added 9 commits June 26, 2020 22:00

Fix string view typedef in sstream.

0aee2b5

Fix build

b8a3009

Fix build.

4975e55

Fix rvalue of string.

99bf2c6

Fix typo

213b078

Pass test.

f056152

Fix typo.

9d7726b

Fix for read-write buffer.

662e683

Fix for fancy pointer.

66b6074

This comment has been minimized.

Sign in to view

Berrysoft mentioned this pull request Jun 30, 2020

<sstream>: Change the underlying buffer of basic_stringbuf to basic_string #938

Open

StephanTLavavej requested changes Jun 30, 2020

View reviewed changes

Berrysoft added 2 commits June 30, 2020 15:51

Fix for 27 requested changes.

8797986

Use _STL_INTERNAL_CHECK

038f6dd

miscco reviewed Jun 30, 2020

View reviewed changes

stl/inc/sstream Outdated Show resolved Hide resolved

Berrysoft added 3 commits June 30, 2020 18:12

Use _Prev_iter

325427e

Add test for empty string.

167e601

Clearify noexcept

f8fbede

This comment has been minimized.

Sign in to view

StephanTLavavej reviewed Jan 14, 2021

View reviewed changes

StephanTLavavej added 15 commits January 14, 2021 15:39

Merge branch 'master' into stringbuf_view

59d4ebe

Merge branch 'master' into stringbuf_view

f4ab8e2

Drop std::.

94bbb1c

Remove allocator data member.

9611287

Add missing includes.

efac7f0

Other code review feedback for the test.

fd70b56

Strengthen noexcept on swap.

0de5a7e

Add missing explicit.

f3a0dee

Fix ios_base::in/out flags.

776ceab

For basic_istringstream/basic_ostringstream, this fixes correctness. For basic_stringstream, this improves clarity and debug codegen.

Rename to _Size, _Res.

71181c6

Drop unnecessary static_cast<allocator_type>.

9a47f73

Improve comments.

a962d95

Use empty braces to construct temporaries.

2d7f2b1

Use if constexpr to improve assertion.

352c60e

Overhaul constraints.

f1073f4

StephanTLavavej approved these changes Jan 15, 2021

View reviewed changes

StephanTLavavej removed their assignment Jan 15, 2021

StephanTLavavej removed the request for review from BillyONeal January 15, 2021 08:14

StephanTLavavej self-assigned this Jan 15, 2021

StephanTLavavej merged commit 6c9f6da into microsoft:master Jan 16, 2021

Berrysoft deleted the stringbuf_view branch January 16, 2021 03:42

StephanTLavavej mentioned this pull request Mar 17, 2021

Avoid concepts when __cpp_lib_concepts isn't defined #1749

Merged

frederick-vs-ja mentioned this pull request Sep 3, 2024

<string>, <sstream>: Is the _String_constructor_rvalue_allocator_tag internal constructor necessary? #4929

Closed

Efficient Access To basic_stringbuf's Buffer #919

Efficient Access To basic_stringbuf's Buffer #919

Conversation

Berrysoft commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Berrysoft commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

StephanTLavavej Jan 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

StephanTLavavej Jan 13, 2021

Choose a reason for hiding this comment

Uh oh!

StephanTLavavej Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

StephanTLavavej Jan 13, 2021

Choose a reason for hiding this comment

Uh oh!

StephanTLavavej Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Berrysoft commented Jan 15, 2021

Uh oh!

StephanTLavavej commented Jan 15, 2021

Uh oh!

StephanTLavavej commented Jan 16, 2021

Uh oh!

Uh oh!

Berrysoft commented Jun 25, 2020 •

edited

Loading

Berrysoft commented Jun 26, 2020 •

edited

Loading