operator string::string_view may be dangerous

[Konstantin Tenzin]

Hi all,

Following code may cause runtime error:

std::string readSettingFromFile(const std::string& fileName);
std::string_view getSetting()
{
  std::string fileName = "settings.ini";
  auto setting = readSettingFromFile(fileName);
  return setting;
}

int main()
{
  auto setting = getSetting();
  do_something(setting);  // problem
}

Provided operator gives some flexibility, but looks pretty dangerous. What do you think, is it right to have it in standard?

[Nicol Bolas]

Yes, this is correct.

You ultimately have to have a balance between potentially dangerous things and perfectly reasonable things. At some point, programmers have to take responsibility for the correctness of their own code in order to have useful features.

The code would have been broken if you had returned a `string&` from the function instead of a `string_view`.

[Konstantin Tenzin]

пятница, 24 марта 2017 г., 22:19:58 UTC+3 пользователь Nicol Bolas написал:

 I see your point. In C++ we care about performance. From the other side, it is possible to have reasonably-looking refactoring which breaks application without being noticed by the compiler. Consider following code evolution:

// hello.h
std::string_view hello();          // string_view is perfect, as data is stored in static buffer

// say.h
void say(std::string_view something);

// say_hi.cpp
std::string_view hi = hello();
say(hi);

1 year later => change return type of function 'hello' from string_view to string
// hello.h
// return type changed from std::string_view to std::string. Now it says hello differently depending on the user locale.
// compiler will warn us about potential invalid usage, right?
std::string hello();          

Code compiles, but crashes in say_hi.cpp. WAT???

It's hard to blame developer for changing return value from string_view to string. They are both "strings", both passed by value. Compilation passed, and refactoring does not look too risky. It looks scary.

[Nicol Bolas]

No, it's very easy to blame developers for that.

Refactoring of that nature always has to be done carefully. You cannot just change one type to another without any thought to the consequences of it. That's why refactoring in C++ is best done by creating new functions and deprecating old ones, rather than modifying the signatures of existing APIs.

Anytime you change function signatures, you are opening yourself up to all kinds of non-compatible breakage. Even changing a `const string&` to a `string` return value can break existing code.

[Konstantin Tenzin]

суббота, 25 марта 2017 г., 4:25:19 UTC+3 пользователь Nicol Bolas написал:

It is very hard to agree with this point of view. I can hardly imagine so correct user code, which is so clean and simple, that would break and become unnoticed by the compiler after the changing return value from 'const string&' to 'string'.

[Nicol Bolas]

On Friday, March 24, 2017 at 10:33:10 PM UTC-4, Konstantin Tenzin wrote:

суббота, 25 марта 2017 г., 4:25:19 UTC+3 пользователь Nicol Bolas написал:

On Friday, March 24, 2017 at 7:32:50 PM UTC-4, Konstantin Tenzin wrote:

пятница, 24 марта 2017 г., 22:19:58 UTC+3 пользователь Nicol Bolas написал:

It's hard to blame developer for changing return value from string_view to string. They are both "strings", both passed by value. Compilation passed, and refactoring does not look too risky. It looks scary.

No, it's very easy to blame developers for that.

Refactoring of that nature always has to be done carefully. You cannot just change one type to another without any thought to the consequences of it. That's why refactoring in C++ is best done by creating new functions and deprecating old ones, rather than modifying the signatures of existing APIs.

Anytime you change function signatures, you are opening yourself up to all kinds of non-compatible breakage. Even changing a `const string&` to a `string` return value can break existing code.

It is very hard to agree with this point of view. I can hardly imagine so correct user code, which is so clean and simple, that would break and become unnoticed by the compiler after the changing return value from 'const string&' to 'string'.

Ironically, this very thread provides the perfect example:

string_view sv = object.get_string();

If `get_string` returned a `const string &`, a reference to a string stored in `object`, then that's perfectly functional code. If `get_string` returns a `string` by copy, then this is not functional code. It's well-formed, but using `sv` causes undefined behavior, and for the very reason that this thread exists.

But if you consider that out-of-bounds, since it's the very thing you want changed, consider any number of perfectly legitimate alternatives:

auto sv = string_view(object.get_string()); //I want to *explicitly* convert it to a string_view.
auto ptr = object.get_string().c_str(); //I want to get the data from it.
for(auto ch: object.get_string()); //I want to iterate over the characters in the string.

Every one of these was perfectly valid and reasonable code when it returns a `const&` to something stored in `object`. But it turns bad when it returns a value.

Modifying existing APIs without causing breakage in C++ is very hard. Transforming reference return values into prvalues is not a change you can make without consequence. And `string_view` contains a reference to a `string`, so the same issues apply.

[Matthew Woehlke]

On 2017-03-24 23:59, Nicol Bolas wrote:
> On Friday, March 24, 2017 at 10:33:10 PM UTC-4, Konstantin Tenzin wrote:
>> суббота, 25 марта 2017 г., 4:25:19 UTC+3 пользователь Nicol Bolas написал:

>>> Anytime you change function signatures, you are opening yourself up to
>>> all kinds of non-compatible breakage. Even changing a `const string&` to a
>>> `string` return value can break existing code.
>>>
>>
>> It is very hard to agree with this point of view. I can hardly imagine so
>> correct user code, which is so clean and simple, that would break and
>> become unnoticed by the compiler after the changing return value from
>> 'const string&' to 'string'.
>
> Ironically, this very thread provides the perfect example:
>
> string_view sv = object.get_string();
>
> If `get_string` returned a `const string &`, a reference to a string stored
> in `object`, then that's perfectly functional code. If `get_string` returns
> a `string` by copy, then this is not functional code. It's well-formed, but
> using `sv` causes undefined behavior, and for the very reason that this
> thread exists.

Wait, string_view(string&&) is not deleted?! I really, really hope that
is wrong, and the above would be a hard compile error!

> But if you consider that out-of-bounds, since it's the very thing you want
> changed, consider any number of perfectly legitimate alternatives:
>
> auto sv = string_view(object.get_string()); //I want to *explicitly*
> convert it to a string_view.

Same thing; this should be a hard error.

> auto ptr = object.get_string().c_str(); //I want to get the data from it.

So should this. (Um... is overloading on `this&&` not possible like it
is for `this / this const`? I thought it was, but GCC seems to disagree...)

> for(auto ch: object.get_string()); //I want to iterate over the characters
> in the string.

I think this is okay?

--
Matthew

[Ville Voutilainen]

On 27 March 2017 at 17:46, Matthew Woehlke <mwoehlk...@gmail.com> wrote:
> Wait, string_view(string&&) is not deleted?! I really, really hope that

There is no such thing as string_view(string&&). string_view doesn't
have a constructor that takes
a string, string has a conversion operator that returns a string_view.

> is wrong, and the above would be a hard compile error!

That wouldn't be distinguishable from

void f(string_view);
f(string(whatever));

which we presumably want to keep valid.

>> auto sv = string_view(object.get_string()); //I want to *explicitly*
>> convert it to a string_view.
>
> Same thing; this should be a hard error.

Which would again prevent using the conversion in valid argument-passing cases.

>> auto ptr = object.get_string().c_str(); //I want to get the data from it.
> So should this. (Um... is overloading on `this&&` not possible like it
> is for `this / this const`? I thought it was, but GCC seems to disagree...)

Yes, it's possible. I don't know why you think GCC disagrees with it.

[Matthew Woehlke]

On 2017-03-27 10:51, Ville Voutilainen wrote:

> On 27 March 2017 at 17:46, Matthew Woehlke wrote:
>> Wait, string_view(string&&) is not deleted?! I really, really hope that
>
> There is no such thing as string_view(string&&). string_view doesn't
> have a constructor that takes
> a string, string has a conversion operator that returns a string_view.

Well, moral equivalent then :-).

>> is wrong, and the above would be a hard compile error!
>
> That wouldn't be distinguishable from
>
> void f(string_view);
> f(string(whatever));
>
> which we presumably want to keep valid.

Ack, that's most unfortunate. It's unfortunate we (apparently?) lack the
granularity to delete functions that take or return a reference to an
object's internal state when we know that object is going to cease to
exist before we can make use of the result.

>>> auto ptr = object.get_string().c_str(); //I want to get the data from it.
>> So should this. (Um... is overloading on `this&&` not possible like it
>> is for `this / this const`? I thought it was, but GCC seems to disagree...)
>
> Yes, it's possible. I don't know why you think GCC disagrees with it.

Maybe I am doing it wrong. I expected this to work:

class SomeType
{
...
void func() { ... };
void func() && = delete;
}

...but GCC (4.9.2) says I can't overload like that.

--
Matthew

[Ville Voutilainen]

On 27 March 2017 at 18:57, Matthew Woehlke <mwoehlk...@gmail.com> wrote:
>>>> auto ptr = object.get_string().c_str(); //I want to get the data from it.
>>> So should this. (Um... is overloading on `this&&` not possible like it
>>> is for `this / this const`? I thought it was, but GCC seems to disagree...)
>>
>> Yes, it's possible. I don't know why you think GCC disagrees with it.
>
> Maybe I am doing it wrong. I expected this to work:
>
> class SomeType
> {
> ...
> void func() { ... };
> void func() && = delete;
> }
>
> ...but GCC (4.9.2) says I can't overload like that.

That's because you need

void func() & { ... };

[Nicol Bolas]

On Monday, March 27, 2017 at 10:46:29 AM UTC-4, Matthew Woehlke wrote:

On 2017-03-24 23:59, Nicol Bolas wrote:
> On Friday, March 24, 2017 at 10:33:10 PM UTC-4, Konstantin Tenzin wrote:
>> суббота, 25 марта 2017 г., 4:25:19 UTC+3 пользователь Nicol Bolas написал:
>>> Anytime you change function signatures, you are opening yourself up to
>>> all kinds of non-compatible breakage. Even changing a `const string&` to a
>>> `string` return value can break existing code.
>>>
>>
>> It is very hard to agree with this point of view. I can hardly imagine so
>> correct user code, which is so clean and simple, that would break and
>> become unnoticed by the compiler after the changing return value from
>> 'const string&' to 'string'.
>
> Ironically, this very thread provides the perfect example:
>
> string_view sv = object.get_string();
>
> If `get_string` returned a `const string &`, a reference to a string stored
> in `object`, then that's perfectly functional code. If `get_string` returns
> a `string` by copy, then this is not functional code. It's well-formed, but
> using `sv` causes undefined behavior, and for the very reason that this
> thread exists.

Wait, string_view(string&&) is not deleted?! I really, really hope that
is wrong, and the above would be a hard compile error!

I don't disagree. But that's how it currently is. And changing it now would be backwards-incompatible.

> for(auto ch: object.get_string()); //I want to iterate over the characters
> in the string.

I think this is okay?

Yeah, I forgot about lifetime extension of `auto&&` there.

[Richard Hodges]

> I don't disagree. But that's how it currently is. And changing it now would be backwards-incompatible.

incompatible with what? std::string_view is not in wide use. In any case, deprecation within the standard is legal and has precedent.

--

---
You received this message because you are subscribed to the Google Groups "ISO C++ Standard - Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to std-discussion+unsubscribe@isocpp.org.
To post to this group, send email to std-dis...@isocpp.org.
Visit this group at https://groups.google.com/a/isocpp.org/group/std-discussion/.

[Nicol Bolas]

On Monday, March 27, 2017 at 2:22:48 PM UTC-4, Richard Hodges wrote:

> I don't disagree. But that's how it currently is. And changing it now would be backwards-incompatible.

incompatible with what?

C++17, which is, as I understand it, essentially done.

You might get it through with a defect report, but since the resolution would be non-BC, I'm not sure exactly how well it would go.

[Olaf van der Spek]

Op maandag 27 maart 2017 18:11:04 UTC+2 schreef Ville Voutilainen:

What's the difference between func() {} and func() & {}?

I thought there were 'just' three options, const, non-const and &&.

[Olaf van der Spek]

Op maandag 27 maart 2017 17:58:02 UTC+2 schreef Matthew Woehlke:

> That wouldn't be distinguishable from
>
> void f(string_view);
> f(string(whatever));
>
> which we presumably want to keep valid.

Ack, that's most unfortunate. It's unfortunate we (apparently?) lack the
granularity to delete functions that take or return a reference to an
object's internal state when we know that object is going to cease to
exist before we can make use of the result.

 

It's a bug, not a feature. ;) 

Why would you want to disallow the above construct, which works perfectly fine?

[Ville Voutilainen]

On 29 March 2017 at 10:18, Olaf van der Spek <olafv...@gmail.com> wrote:
>> That's because you need
>>
>> void func() & { ... };
>> void func() && = delete;
>
>
> What's the difference between func() {} and func() & {}?

The former can be called with an lvalue or an rvalue, the latter only
with an lvalue.
If overloading func(); with a ref-qualified overload were allowed, all
calls would be
ambiguous, which is why it's rejected at declaration time.

> I thought there were 'just' three options, const, non-const and &&.

I wouldn't want to speculate what that misunderstanding is based on.