6 ms·

> It's the first time I'm hearing this Say that you have some `Person` class with a `get_name()` method that returns the person's name. Then maybe you write s

by abbeyj 4y ago

> It's the first time I'm hearing this Say that you have some `Person` class with a `get_name()` method that returns the person's name. Then maybe you write some code like this to use it: Person person; // ... std::string_view name(person.get_name()); // Use `name` here Is this code safe? Well it depends on the implementation of `Person::get_name`. Maybe it looks like this: std::string& Person::get_name() { return m_name; } Then the code should be OK because the call will return a reference to a member of `person` and `person` will outlive `name`. But what if instead the implementation looks like this: std::string Person::get_name() { return m_first_name + " " + m_last_name; } Now you're in trouble. A temporary string will be created and the string_view will refer to that string. That temporary gets destroyed at the end of the statement and then `name` is dangling. By the time you get to use `name` it is already broken. It is very easy to make this kind of mistake. Much easier than with pointers IMO since you usually have to explicitly take a pointer to something and you can't directly get a pointer to a temporary (`std::string* p = &person.get_name()` is an error if `Person::get_name` returns `std::string`). By contrast, getting a const reference to a temporary and then constructing a string_view out of it is not an error or even a warning. It is all done implicitly so there is no indication in the code that it is even happening. The only difference between code that is correct and code that is incorrect is a single ampersand far away in some header file. This type of mistake probably won't get caught in code review. How often does a person really go track down and examine a header file when reviewing a pull request? Probably they're only going to look at the actual files that are being changed. Even worse, say that `Person::get_name` is using the first implementation so the code is correct and working. But later on somebody does a refactor such `Person::get_name` uses the second implementation. Now they've broken things but they're unaware of this fact. They want to be responsible and find any problems that they might have introduced. The first step is to run a build and see if there are any complaints from the compiler. That is successful and the compiler produces no warnings. Encouraged by this they then run all the tests. And all the tests pass! Reading memory just after it has been freed will often give you back the last thing that was stored there so the tests see the values that they expect. So with a successful build, a successful run of all the tests, and no problems spotted in code review, they confidently merge the change. And now you've got a use-after-free in your code that will eventually cause a problem. Yes, using Address Sanitizer will catch this. But it is still a pretty easy way to introduce a bug into your program. Code that does not look at all suspicious can be completely broken.

blub 4y ago
I would flag assigning a return value to a string_view in code review. Same as assigning to a pointer. Yes, it’s one more thing nudging the programmer toward unsafe use, but this is C++ we’re talking about: only the strong survive.
electrograv 4y ago
The root cause of the danger you illustrate here IMO is actually a fault in the way the `std::string` to `std::string_view` implicit conversion is designed, as opposed to something inherently wrong with the concept of view/span classes. In my opinion, `string_view` and `span` are otherwise wonderfully much-needed concepts (that arguably should have been built into the language itself, like Rust slices) that should never have been designed to allow implicitly creating views of temporaries. Disabling implicit conversion/construction from rvalue references is quite possible in C++17 and beyond, and very effectively prevents accidentally implicitly viewing temporaries just fine (I’ve done this myself in some enhanced span-like classes I’ve written), so I’m really not sure why string_view was designed this way. https://en.cppreference.com/w/cpp/string/basic_string/operator_basic_string_view https://en.cppreference.com/w/cpp/string/basic_string/operat...
- abbeyj 4y ago
  I think that would be safer. But then it would presumably disallow things like: void frob(std::string_view sv); std::string a = "a"; std::string b = "b"; frob(a + b); This is allowed today and does not have any problems that I'm aware of. You could work around this by changing the call to: frob(std::string_view(a + b)); But that feels cumbersome. I think the goal of std::string_view was to be useful as a parameter for a function so that you could pass in anything remotely string-like and it would implicitly convert and do what you meant. Requiring explicit conversions at the call sites would go against that goal. Another thing that was desired was the ability to take an existing function that takes a `const std::string&` and change it to instead take a `std::string_view` and not have to update any of the callers. If some callers that worked with the old function are going to need an update to work with the new function then it gets a lot harder to change the function. Or maybe impossible if you don't control all of the callers. I don't see how you can get both the ease-of-use and the safety short of reinventing something like Rust's lifetimes.

by abbeyj 4y ago

blub 4y ago

I would flag assigning a return value to a string_view in code review. Same as assigning to a pointer. Yes, it’s one more thing nudging the programmer toward unsafe use, but this is C++ we’re talking about: only the strong survive.

electrograv 4y ago

The root cause of the danger you illustrate here IMO is actually a fault in the way the `std::string` to `std::string_view` implicit conversion is designed, as opposed to something inherently wrong with the concept of view/span classes. In my opinion, `string_view` and `span` are otherwise wonderfully much-needed concepts (that arguably should have been built into the language itself, like Rust slices) that should never have been designed to allow implicitly creating views of temporaries. Disabling implicit conversion/construction from rvalue references is quite possible in C++17 and beyond, and very effectively prevents accidentally implicitly viewing temporaries just fine (I’ve done this myself in some enhanced span-like classes I’ve written), so I’m really not sure why string_view was designed this way. https://en.cppreference.com/w/cpp/string/basic_string/operator_basic_string_view https://en.cppreference.com/w/cpp/string/basic_string/operat...

abbeyj 4y ago
I think that would be safer. But then it would presumably disallow things like: void frob(std::string_view sv); std::string a = "a"; std::string b = "b"; frob(a + b); This is allowed today and does not have any problems that I'm aware of. You could work around this by changing the call to: frob(std::string_view(a + b)); But that feels cumbersome. I think the goal of std::string_view was to be useful as a parameter for a function so that you could pass in anything remotely string-like and it would implicitly convert and do what you meant. Requiring explicit conversions at the call sites would go against that goal. Another thing that was desired was the ability to take an existing function that takes a `const std::string&` and change it to instead take a `std::string_view` and not have to update any of the callers. If some callers that worked with the old function are going to need an update to work with the new function then it gets a lot harder to change the function. Or maybe impossible if you don't control all of the callers. I don't see how you can get both the ease-of-use and the safety short of reinventing something like Rust's lifetimes.