Should boost::core::string_view be made public?

older
Re: [boost] [Scope] Peer review...

Дмитрий Архипов

8 Feb 2024 8 Feb '24

11:28 a.m.

boost::core::string_view is a string_view implementation that (unlike boost::string_view from Utility) can interconvert with std::string_view. This functionality was necessary for Boost.JSON to drop the standalone mode, because people who have access to C++17 prefer to use std::string_view wherever they can. At the time there was no consensus on whether the type should be made public. Now in addition to Boost.JSON the type is also used at least by Boost.URL. And recently added Charconv is also using it. So, I want to raise the question again: should we make the type public and refer to it directly in our libraries' docs, rather than considering it an implementation detail?

Show replies by date

Andrey Semashev

8 Feb 8 Feb

11:45 a.m.

On 2/8/24 14:28, Дмитрий Архипов via Boost wrote:

...

boost::core::string_view is a string_view implementation that (unlike boost::string_view from Utility) can interconvert with std::string_view. This functionality was necessary for Boost.JSON to drop the standalone mode, because people who have access to C++17 prefer to use std::string_view wherever they can. At the time there was no consensus on whether the type should be made public.

Now in addition to Boost.JSON the type is also used at least by Boost.URL. And recently added Charconv is also using it. So, I want to raise the question again: should we make the type public and refer to it directly in our libraries' docs, rather than considering it an implementation detail?

I think, if boost::core::string_view is to become public, it should come along with deprecating and then removing boost::string_view from Boost.Utility. As well as boost::string_ref. Having three string_view classes (if you count boost::string_ref, four if you count std::string_view) is a real problem for interoperability. This is assuming the two implementations are compatible in terms of interface and behavior. I'll add that this will not be a free transition for other libraries that support or use string views. For example, in Boost.Log I support boost::string_view and boost::string_ref but not boost::core::string_view. Not that it should be difficult to switch, but it's work. It's more complicated if the library or tool is unmaintained.

Andrey Semashev

11:54 a.m.

On 2/8/24 14:45, Andrey Semashev wrote:

...

On 2/8/24 14:28, Дмитрий Архипов via Boost wrote:

...
boost::core::string_view is a string_view implementation that (unlike boost::string_view from Utility) can interconvert with std::string_view. This functionality was necessary for Boost.JSON to drop the standalone mode, because people who have access to C++17 prefer to use std::string_view wherever they can. At the time there was no consensus on whether the type should be made public.

Now in addition to Boost.JSON the type is also used at least by Boost.URL. And recently added Charconv is also using it. So, I want to raise the question again: should we make the type public and refer to it directly in our libraries' docs, rather than considering it an implementation detail?

I think, if boost::core::string_view is to become public, it should come along with deprecating and then removing boost::string_view from Boost.Utility. As well as boost::string_ref. Having three string_view classes (if you count boost::string_ref, four if you count std::string_view) is a real problem for interoperability.

This is assuming the two implementations are compatible in terms of interface and behavior.

Which they are not, as boost::core::string_view doesn't support char_traits. So it looks like a non-starter to me.

...

I'll add that this will not be a free transition for other libraries that support or use string views. For example, in Boost.Log I support boost::string_view and boost::string_ref but not boost::core::string_view. Not that it should be difficult to switch, but it's work. It's more complicated if the library or tool is unmaintained.

Дмитрий Архипов

12:59 p.m.

чт, 8 февр. 2024 г. в 14:54, Andrey Semashev via Boost :

...

...
This is assuming the two implementations are compatible in terms of interface and behavior.

Which they are not, as boost::core::string_view doesn't support char_traits. So it looks like a non-starter to me.

Which is why I did not suggest deprecating boost::basic_string_view.

Peter Dimov

3:14 p.m.

Andrey Semashev wrote:

...

Which they are not, as boost::core::string_view doesn't support char_traits. So it looks like a non-starter to me.

Do you really need or support traits other than std::char_traits<Ch>? Why and where?

Andrey Semashev

3:26 p.m.

On 2/8/24 18:14, Peter Dimov via Boost wrote:

...

Andrey Semashev wrote:

...
Which they are not, as boost::core::string_view doesn't support char_traits. So it looks like a non-starter to me.

Do you really need or support traits other than std::char_traits<Ch>? Why and where?

It's not that I need it. It's that std::basic_string(_view) are defined this way (for better or worse), and for our string_view to properly emulate and inperoperate with std::string_view, it should have it as well. You're not winning anything by not supporting it either as you're using std::char_traits internally anyway.

Peter Dimov

3:54 p.m.

Andrey Semashev wrote:

...

On 2/8/24 18:14, Peter Dimov via Boost wrote:

...
Andrey Semashev wrote:

...
Which they are not, as boost::core::string_view doesn't support char_traits. So it looks like a non-starter to me.

Do you really need or support traits other than std::char_traits<Ch>? Why and where?

It's not that I need it. It's that std::basic_string(_view) are defined this way (for better or worse), and for our string_view to properly emulate and inperoperate with std::string_view, it should have it as well.

That's not true at all.

...

You're not winning anything by not supporting it either as you're using std::char_traits internally anyway.

core::string_view is intended to be used in this manner: void my_api_function( core::string_view sv ); which then allows the user to pass std::string, std::string_view, boost::string_view. It's not intended to be used in this manner template<class Ch> void my_api_function( core::basic_string_view<Ch> sv ); because then conversions don't work. There is no benefit in also supporting template void my_api_function( core::basic_string_view sv ); because it doesn't work either.

Andrey Semashev

4:11 p.m.

On 2/8/24 18:54, Peter Dimov via Boost wrote:

...

Andrey Semashev wrote:

...
On 2/8/24 18:14, Peter Dimov via Boost wrote:

...
Andrey Semashev wrote:

...
Which they are not, as boost::core::string_view doesn't support char_traits. So it looks like a non-starter to me.

Do you really need or support traits other than std::char_traits<Ch>? Why and where?

It's not that I need it. It's that std::basic_string(_view) are defined this way (for better or worse), and for our string_view to properly emulate and inperoperate with std::string_view, it should have it as well.

That's not true at all.

...
You're not winning anything by not supporting it either as you're using std::char_traits internally anyway.

core::string_view is intended to be used in this manner:

void my_api_function( core::string_view sv );

which then allows the user to pass std::string, std::string_view, boost::string_view.

It's not intended to be used in this manner

template<class Ch> void my_api_function( core::basic_string_view<Ch> sv );

because then conversions don't work.

There is no benefit in also supporting

template void my_api_function( core::basic_string_view sv );

because it doesn't work either.

This implies that the API only works with one character type, which is not always the case, as shown in Boost.Log. Template interface parity is useful in cases like this: https://github.com/boostorg/container/blob/6e697d796897b32b471b4f0740dcaa03d... https://github.com/boostorg/container/blob/6e697d796897b32b471b4f0740dcaa03d... It also allows to interoperate with strings with custom char traits just in case if someone actually uses them.

Peter Dimov

4:24 p.m.

Andrey Semashev wrote:

...

This implies that the API only works with one character type, which is not always the case, as shown in Boost.Log.

It implies that if you want to accept e.g. wchar_t, you need a separate overload.

...

Template interface parity is useful in cases like this:

https://github.com/boostorg/container/blob/6e697d796897b32b471b4f0740dcaa03d...

https://github.com/boostorg/container/blob/6e697d796897b32b471b4f0740dcaa03d...

This accepts vector, but this might be considered a feature. :-)

Andrey Semashev

4:36 p.m.

On 2/8/24 19:24, Peter Dimov via Boost wrote:

...

Andrey Semashev wrote:

...
This implies that the API only works with one character type, which is not always the case, as shown in Boost.Log.

It implies that if you want to accept e.g. wchar_t, you need a separate overload.

This doesn't scale very well. Imagine the amount of overloads in something like boost::filesystem::path. (Yes, I know it currently supports char and wchar_t, but I've been asked to support other character types as well.)

...

...
Template interface parity is useful in cases like this:

https://github.com/boostorg/container/blob/6e697d796897b32b471b4f0740dcaa03d...

https://github.com/boostorg/container/blob/6e697d796897b32b471b4f0740dcaa03d...

This accepts vector, but this might be considered a feature. :-)

:) Yes, but my point is that it currently doesn't work with boost::core::string_view.

Peter Dimov

4:51 p.m.

Andrey Semashev wrote:

...

On 2/8/24 19:24, Peter Dimov via Boost wrote:

...
Andrey Semashev wrote:

...
This implies that the API only works with one character type, which is not always the case, as shown in Boost.Log.

It implies that if you want to accept e.g. wchar_t, you need a separate overload.

This doesn't scale very well. Imagine the amount of overloads in something like boost::filesystem::path. (Yes, I know it currently supports char and wchar_t, but I've been asked to support other character types as well.)

Whether it scales or not, this is the intended use of core::string_view. As I said, if you make the function a template, conversions no longer work, which defeats the purpose of using core::string_view.

Vinnie Falco

9:57 p.m.

On Thu, Feb 8, 2024 at 8:36 AM Andrey Semashev via Boost < boost@lists.boost.org> wrote:

...

...but I've been asked to support other character types as well.)

You say YES to those requests. I say NO to such requests. We are not the same. Thanks

Дмитрий Архипов

4:57 p.m.

чт, 8 февр. 2024 г. в 19:11, Andrey Semashev via Boost :

...

Template interface parity is useful in cases like this:

https://github.com/boostorg/container/blob/6e697d796897b32b471b4f0740dcaa03d...

https://github.com/boostorg/container/blob/6e697d796897b32b471b4f0740dcaa03d...

std::basic_string instead uses this check:

...

This overload participates in overload resolution only if std::is_convertible_v> is true and std::is_convertible_v is false.

Peter Dimov

5:03 p.m.

??????? ??????? (but it was actually Dmitry Arkhipov) wrote:

...

чт, 8 февр. 2024 г. в 19:11, Andrey Semashev via Boost :

...
Template interface parity is useful in cases like this:

https://github.com/boostorg/container/blob/6e697d796897b32b471b4f074 0d

...
caa03d8ee57cc/include/boost/container/string.hpp#L685-L691

https://github.com/boostorg/container/blob/6e697d796897b32b471b4f074 0d

...
caa03d8ee57cc/include/boost/container/string.hpp#L2314-L2316

std::basic_string instead uses this check:

...
This overload participates in overload resolution only if std::is_convertible_v> is true and std::is_convertible_v is false.

I prefer to detect StringLike types by checking for T::value_type, T::traits_type, T::data() and T::size(). That's almost what the range constructor of std::string_view does, except the check for T::traits_type was not introduced there because the constructor was made explicit instead.

Peter Dimov

3:10 p.m.

Andrey Semashev wrote:

...

I'll add that this will not be a free transition for other libraries that support or use string views. For example, in Boost.Log I support boost::string_view and boost::string_ref but not boost::core::string_view. Not that it should be difficult to switch, but it's work. It's more complicated if the library or tool is unmaintained.

core::string_view is intended to be the single type you need to support. It converts from (and to) boost::string_view in addition to std::string_view. (Not boost::string_ref though, which should be obsolete at this point. And deprecated.)

Andrey Semashev

3:23 p.m.

On 2/8/24 18:10, Peter Dimov via Boost wrote:

...

Andrey Semashev wrote:

...
I'll add that this will not be a free transition for other libraries that support or use string views. For example, in Boost.Log I support boost::string_view and boost::string_ref but not boost::core::string_view. Not that it should be difficult to switch, but it's work. It's more complicated if the library or tool is unmaintained.

core::string_view is intended to be the single type you need to support. It converts from (and to) boost::string_view in addition to std::string_view.

No, conversion won't work as I need to detect string types and process them specially (perform character code conversion). And for arbitrary types I need to forward to the type's own operator<<. https://github.com/boostorg/log/blob/develop/include/boost/log/utility/forma... https://github.com/boostorg/log/blob/develop/include/boost/log/utility/forma...

Peter Dimov

4:01 p.m.

Andrey Semashev wrote:

...

On 2/8/24 18:10, Peter Dimov via Boost wrote:

...
Andrey Semashev wrote:

...
I'll add that this will not be a free transition for other libraries that support or use string views. For example, in Boost.Log I support boost::string_view and boost::string_ref but not boost::core::string_view. Not that it should be difficult to switch, but it's work. It's more complicated if the library or tool is unmaintained.

core::string_view is intended to be the single type you need to support. It converts from (and to) boost::string_view in addition to std::string_view.

No, conversion won't work as I need to detect string types and process them specially (perform character code conversion). And for arbitrary types I need to forward to the type's own operator<<.

https://github.com/boostorg/log/blob/develop/include/boost/log/utility/forma...

https://github.com/boostorg/log/blob/develop/include/boost/log/utility/forma...

Yes, I see. I agree that core::string_view isn't going to simplify this code. I note that you're throwing away OtherTraitsT at earliest opportunity, which almost certainly means you're copying values of OtherCharT without going through OtherTraitsT::copy and OtherTraitsT::assign. Is that legal? Nobody knows or cares because nobody uses traits other than std::char_traits. (Well, maybe stdlib implementers do know and do use them in test suites.) The sooner traits type die, the better. Unfortunately that would be never.

Andrey Semashev

4:22 p.m.

On 2/8/24 19:01, Peter Dimov via Boost wrote:

...

Andrey Semashev wrote:

...
On 2/8/24 18:10, Peter Dimov via Boost wrote:

...
Andrey Semashev wrote:

...
I'll add that this will not be a free transition for other libraries that support or use string views. For example, in Boost.Log I support boost::string_view and boost::string_ref but not boost::core::string_view. Not that it should be difficult to switch, but it's work. It's more complicated if the library or tool is unmaintained.

core::string_view is intended to be the single type you need to support. It converts from (and to) boost::string_view in addition to std::string_view.

No, conversion won't work as I need to detect string types and process them specially (perform character code conversion). And for arbitrary types I need to forward to the type's own operator<<.

https://github.com/boostorg/log/blob/develop/include/boost/log/utility/forma...

https://github.com/boostorg/log/blob/develop/include/boost/log/utility/forma...

Yes, I see. I agree that core::string_view isn't going to simplify this code.

I note that you're throwing away OtherTraitsT at earliest opportunity, which almost certainly means you're copying values of OtherCharT without going through OtherTraitsT::copy and OtherTraitsT::assign. Is that legal? Nobody knows or cares because nobody uses traits other than std::char_traits.

I'm not copying or assigning characters within the argument string, so I don't think I'm supposed to use OtherTraitsT.

...

(Well, maybe stdlib implementers do know and do use them in test suites.)

The sooner traits type die, the better. Unfortunately that would be never.

Daniele Lupo

11:58 a.m.

My two cents is that there are already a lot of strings and string_view implementation out there. Exposing this does not add anything, but adds only another type of string that must be handled. On 08/02/2024 12:28, Дмитрий Архипов via Boost wrote:

...

boost::core::string_view is a string_view implementation that (unlike boost::string_view from Utility) can interconvert with std::string_view. This functionality was necessary for Boost.JSON to drop the standalone mode, because people who have access to C++17 prefer to use std::string_view wherever they can. At the time there was no consensus on whether the type should be made public.

Now in addition to Boost.JSON the type is also used at least by Boost.URL. And recently added Charconv is also using it. So, I want to raise the question again: should we make the type public and refer to it directly in our libraries' docs, rather than considering it an implementation detail?

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

Дмитрий Архипов

12:56 p.m.

чт, 8 февр. 2024 г. в 15:39, Daniele Lupo via Boost :

...

My two cents is that there are already a lot of strings and string_view implementation out there. Exposing this does not add anything, but adds only another type of string that must be handled.

The problem is that it has to be handled anyway, because it is used in the APIs of several Boost libraries. But currently the libraries essentially will have to pretend those are unrelated string_view types or reference an undocumented type from Core.

Andrey Semashev

1:31 p.m.

On 2/8/24 15:56, Дмитрий Архипов via Boost wrote:

...

чт, 8 февр. 2024 г. в 15:39, Daniele Lupo via Boost :

...
My two cents is that there are already a lot of strings and string_view implementation out there. Exposing this does not add anything, but adds only another type of string that must be handled.

The problem is that it has to be handled anyway, because it is used in the APIs of several Boost libraries. But currently the libraries essentially will have to pretend those are unrelated string_view types or reference an undocumented type from Core.

You could document a concept of a string view or at least list the supported string types and use that in the documentation.

Дмитрий Архипов

2:36 p.m.

чт, 8 февр. 2024 г. в 16:31, Andrey Semashev via Boost :

...

You could document a concept of a string view or at least list the supported string types and use that in the documentation.

Well, I could write something like "this is an alias to an unspecified type that satisfies such and such concept", and document the concept. And then every other library that uses core::string_view (which currently are: Beast, MySQL, URL, and soon Charconv) would have to do the same. Instead of 3 documented string view types, Boost would have 7.

292

Age (days ago)

292

Last active (days ago)

List overview

Download

21 comments

5 participants

participants (5)

Andrey Semashev
Daniele Lupo
Peter Dimov
Vinnie Falco
Дмитрий Архипов