4 ms·

> Instead of reducing bugs, these might be introducing new footguns. Same thing could be/has been said about std::string_view or std::span but here we are. L

• dataflow 4y ago
  > Same thing could be/has been said about std::string_view or std::span but here we are. It's the first time I'm hearing this, and it doesn't sound like an opinion I've ever had. Those are just range-checked pointers; their ownership semantics aren't any different from those of raw pointers, and they're no less safe than raw pointers. In contrast these actually do have weird ownership semantics, and they have a propensity to introduce safety bugs into the common use cases that didn't exist before.
  • 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.
  • flohofwoe 4y ago
    When I see a raw pointer in C or C++ code I know automatically that gotchas are involved and I need to be extra careful. When I see std::string_view, nothing tells me that this may lead to memory corruption because the underlying storage may go away at any time (and the compiler will happily let this happen). New C++ features simply should not create new memory corruption footguns in addition to the existing ones, embarrassments like iterator invalidation are already bad enough (and worse than typical C memory management issues, because in C such issues are usually 'in your face', while C++ hides them beneath layers upon layers of stdlib abstractions).
    • dataflow 4y ago
      > When I see std::string_view, nothing tells me that this may lead to memory corruption because the underlying storage may go away at any time That's not true. The word "view" quite explicitly tells you that it's a view... into something else, whose existence is generally independent of you (just like in real life!). If anything, it's clearer than an asterisk. > (and the compiler will happily let this happen). Just like with pointers and iterators. There's no difference here. > When I see a raw pointer in C or C++ code I know automatically that gotchas are involved and I need to be extra careful. You "know" this for iterators too. There's no reason why you can't "know" it for views too. It's not like they're introducing a new ownership concept here - these are just bounded versions of iterators and pointers.
• 112233 4y ago
  And the way lambdas are implemented, that can be type-erased into function<> with no regard to capture-by-reference. C++ is a huge sinking ship...