-
Notifications
You must be signed in to change notification settings - Fork 86
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Showing
4 changed files
with
799 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
95924b1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to say, I'm not a fan of this. Now we have two different
string_view
types, not countingstd::string_view
andboost::string_ref
, when it is supposed to be a universal vocabulary type. :( How is this supposed to coexist withboost::string_view
from Boost.Utility? Have you discussed this with @mclow?95924b1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marshall refuses to add conversions from and to
std::string_view
to "his"string_view
. People have been asking him that for probably a year now, if not more. Libraries need an interoperablestring_view
that they can use without requiring C++17 but which can still accept and convert tostd::string_view
.E.g. if you have the API
people who use C++17 want to be able to invoke
my_function
with astd::string_view
and then store the result in astd::string_view
. With thisstring_view
, they can. Withboost::string_view
, they cannot.95924b1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm aware of that situation, and I also think the conversions would be useful. But that doesn't look like a reason enough to introduce yet another
string_view
. Because now people will need to have this:And yes, full disclosure, I'm going to be affected by this in Boost.Log, and I'm not happy about this.
95924b1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the question above; why would you want to have these?
95924b1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because supposedly the one who writes this API wants to accept any string in
my_function
. If he accepts Booststring_view
s as part of this, it is only logical to accept both now.95924b1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If he accepts Boost
string_view
, then he doesn't acceptstd::string_view
, which means he doesn't accept any string. That's exactly the problem withboost::string_view
that we have and need to solve.I understand that in Boost.Log you'll now need to add more overloads, but I don't think denying everyone else a converting
string_view
is the best solution to that problem. I would instead look into defining anis_string
metafunction. Ironically, the best implementation that comes to mind - checking thatT::traits_type
isstd::char_traits<T>
- disallows arbitrary traits, but there are other options, e.g. checking thatT::traits_type::char_type
isT::value_type
. :-) (You'd also need to verify thatdata()
andsize()
exist and returnT const*
andsize_type
.)95924b1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not arguing because I have to modify Boost.Log, I'm arguing because that change does not make things better - it makes code more complicated and more heavy. And this kind of changes will not be localized to Boost.Log or Boost. Whereas the point of
string_view
is exactly the opposite.He does, though it requires explicit conversion. Yes, a nuicance, but not a killer, and definitely not as much of a problem as adding yet another overload. Which is incompatible with
boost::string_view
andstd::string_view
anyway because of the different template parameters. This is actually a problem for users in Boost as they won't be able to useboost::core::string_view
as a drop-in replacement forstd::string_view
, which is often done to lower C++ version requirements.What I think should be done, is further discussion with @mclow and on the ML, possibly with a vote to force the change in
boost::string_view
as widely requested. I don't think Marshall explained his motivation for his decision, and I suspect he might not have understood what was being proposed (there was a lot of confusion about Boost.Config macros in that PR).95924b1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new library that only accepts and returns
boost::core::string_view
doesn't have this problem. It acceptsstd::string
andstd::string_view
automatically, and it "returns"std::string_view
automatically. No overloads are required.95924b1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose an argument needs to be made here that
boost::core::string_view
should convert from and toboost::string_view
as well, but doing that without including its header is hard without some form of cooperation fromboost::string_view
. (It can't be forward-declared.)95924b1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is arguably a lot of code written for
boost::string_view
, that code won't work with that interface. And some code involving template specializations and templated overloads like in Boost.Log won't ever work withboost::core::string_view
until modified. You can't solve every problem by adding conversions, and multiple string view types is still a problem.BTW, I think you can forward-declare as long as default template arguments match.
95924b1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no better path forward. It's not like I was particularly thrilled by the thought of writing another string_view, complete with a test suite.
Either way, this discussion belongs on the list. I don't mind throwing this entire thing in the trash if needed, as long as a better solution to the problem is offered and implemented.
Compilers disagree. https://godbolt.org/z/EeYoYTG38
95924b1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I posted a message on the ML.
Ok, then you can omit the default: https://godbolt.org/z/bb35xEWGT
95924b1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this might work.