On 17/03/2017 20:29, Simon Sasburg via Boost wrote:
I've just tested our codebase with the 1.64.0 master snapshot, and i've encountered an issue which i've tracked down to this commit:
https://github.com/boostorg/utility/commit/9960d9f395b79ee860e39064cd4696 1f76d2cb55
Basically this removes the constructor which allows constructing a boost::string_ref from an rvalue std::string. I am wondering if this is really that dangerous. In our codebase we almost exlusively use boost::string_ref as function parameters, with the understanding that passed string_refs parameters are only valid within the function body. (so you need to copy it into an std::string if you need to access it afterwards) With these constraints i think it is OK to construct a string_ref from an rvalue std::string because the temporary should live long enough. (but please correct me if i'm wrong here)
If you are calling a function that returns a std::string (thus you have an rvalue temporary) and you are immediately wrapping this in a string_ref (as another temporary) and then passing this as a function parameter, AFAIK the compiler is within its rights to delete the temporary std::string prior to the call as it is not the actual parameter. (To put it more generally: only the temporaries that are the actual parameters are "safe" -- any temporaries used to construct those temporaries might be deleted prior to the call.) Although you might get away with it sometimes (perhaps even most of the time) as it probably wouldn't do so without aggressive optimisation -- but you shouldn't rely on this; it's unsafe usage and subject to the whims of the compiler. The safe version is to store the returned std::string as an lvalue (eg. a local variable) prior to the function call that constructs the temporary rvalue string_refs.
Is the safety (?) added by this commit worth it if it breaks working code?
Can you post an example of such working code that's broken by this change? (Assuming that it's not covered by the above case.)