[Any] inconsistent return types of any_cast overloads
Hello Boost Users, Code like the following used to work with Boost 1.53 on x86-64 Linux using GCC 4.7 and -std=c++11: boost::any makeVec() { return std::vector<int>{ 1, 2, 3 }; } void foo() { const auto& vec = boost::any_cast<std::vector<int>>(makeVec()); for ( auto value : vec ) { // ... } } The same code is likely to cause a segmentation violation using Boost 1.55. According to the online documentation *1, any_cast has two overloads accepting reference arguments: template<typename ValueType> ValueType any_cast(any&); template<typename ValueType> ValueType any_cast(const any&); If these were indeed the only overloads accepting reference arguments, there is no reason why the example code shouldn't work. ValueType any_cast(const any&) should be the preferred overload and it returns a vector<int> temporary whose lifetime is then extended to the end of the block by being bound to a local const reference, as required by the C++ standard. However, in any.hpp, I discovered the following overload, which is only declared if the macro BOOST_NO_CXX11_RVALUE_REFERENCES is not set. I suppose this overload was added since 1.53, or that, previously, it was by default omitted. template<typename ValueType> ValueType&& any_cast(any&&); This has higher precedence binding to the temporary returned from makeVec, and returns an rvalue reference to the vector<int> contained in said temporary, which is then implicitly converted to a const lvalue reference as by the usual language rules. Since the lifetime of the temporary returned by makeVec is NOT extended - this only works for temporaries directly bound to a local const reference, whereas the result of makeVec is "hidden" inside the call to any_cast - the temporary is destroyed before the start of the for loop, which renders vec invalid and the code a denizen of undefined behavior land. So much for the analysis, now for my actual question. :-) Why does the new overload return by reference when the old ones return by value? The rationale for the old overloads was, I believe, "you should have to ask for a reference if you want one", which makes sense given precisely that using references tends to be more error prone if temporaries are involved. So this would be the very reason why Any does not declare: template<typename ValueType> const ValueType& any_cast(const any&); even though it could be implemented as easily as the actual overload. If I really wanted to get an const reference out of any_cast, I could write something like boost::any_cast<const ValueType&>(var); I can't think of a reason to be inconsistent in this regard and return a reference if a temporary is passed to any_cast, instead of requiring a user who really wants any_cast to return an rvalue reference to write any_cast<ValueType&&>(...). That the overload breaks existing code and (due to being undocumented and not checked at compile time *2), silently so, whereas using ValueType any_cast(any&&) would at least fail to do any harm, is, in my opinion, another reason to be sceptical of the current solution. Furthermore, there is a usability reason for preferring ValueType(any &&): If ValueType &&(any &&) is declared and I do NOT want a reference, I need to write more verbose and inelegant code than in the opposite case: // Name the temporary const auto &vecAny = makeVec(); // Now, ValueType&& any_cast(any&&) is eliminated from candidate set (no conversion from an lvalue to an rvalue reference) const auto& vec = boost::any_cast<std::vector<int>>(vecAny); Note that the const is accidental here, using a mutable lvalue would work as well. Looking forward to your opinions, Arne Vogel *1) http://www.boost.org/doc/libs/1_55_0/doc/html/boost/any_cast_idp25247624.htm... *2) A compiler may issue a warning. However, I found that GCC 4.8.1 does not warn, even with -Wall and the definition of any_cast being known at the point of use.
2013/12/2 Arne Vogel <avogel@benocs.com>
Hello Boost Users,
Code like the following used to work with Boost 1.53 on x86-64 Linux using GCC 4.7 and -std=c++11:
boost::any makeVec() { return std::vector<int>{ 1, 2, 3 }; }
void foo() { const auto& vec = boost::any_cast<std::vector<int>>(makeVec()); for ( auto value : vec ) { // ... } }
The same code is likely to cause a segmentation violation using Boost 1.55. According to the online documentation *1, any_cast has two overloads accepting reference arguments:
template<typename ValueType> ValueType any_cast(any&); template<typename ValueType> ValueType any_cast(const any&);
If these were indeed the only overloads accepting reference arguments, there is no reason why the example code shouldn't work. ValueType any_cast(const any&) should be the preferred overload and it returns a vector<int> temporary whose lifetime is then extended to the end of the block by being bound to a local const reference, as required by the C++ standard.
However, in any.hpp, I discovered the following overload, which is only declared if the macro BOOST_NO_CXX11_RVALUE_REFERENCES is not set. I suppose this overload was added since 1.53, or that, previously, it was by default omitted.
template<typename ValueType> ValueType&& any_cast(any&&);
This has higher precedence binding to the temporary returned from makeVec, and returns an rvalue reference to the vector<int> contained in said temporary, which is then implicitly converted to a const lvalue reference as by the usual language rules. Since the lifetime of the temporary returned by makeVec is NOT extended - this only works for temporaries directly bound to a local const reference, whereas the result of makeVec is "hidden" inside the call to any_cast - the temporary is destroyed before the start of the for loop, which renders vec invalid and the code a denizen of undefined behavior land.
So much for the analysis, now for my actual question. :-)
Why does the new overload return by reference when the old ones return by value? The rationale for the old overloads was, I believe, "you should have to ask for a reference if you want one", which makes sense given precisely that using references tends to be more error prone if temporaries are involved. So this would be the very reason why Any does not declare:
template<typename ValueType> const ValueType& any_cast(const any&);
even though it could be implemented as easily as the actual overload. If I really wanted to get an const reference out of any_cast, I could write something like
boost::any_cast<const ValueType&>(var);
I can't think of a reason to be inconsistent in this regard and return a reference if a temporary is passed to any_cast, instead of requiring a user who really wants any_cast to return an rvalue reference to write any_cast<ValueType&&>(...). That the overload breaks existing code and (due to being undocumented and not checked at compile time *2), silently so, whereas using ValueType any_cast(any&&) would at least fail to do any harm, is, in my opinion, another reason to be sceptical of the current solution.
Furthermore, there is a usability reason for preferring ValueType(any &&): If ValueType &&(any &&) is declared and I do NOT want a reference, I need to write more verbose and inelegant code than in the opposite case:
// Name the temporary const auto &vecAny = makeVec(); // Now, ValueType&& any_cast(any&&) is eliminated from candidate set (no conversion from an lvalue to an rvalue reference) const auto& vec = boost::any_cast<std::vector<int>>(vecAny);
Note that the const is accidental here, using a mutable lvalue would work as well.
Looking forward to your opinions, Arne Vogel
*1) http://www.boost.org/doc/libs/1_55_0/doc/html/boost/any_ cast_idp25247624.html *2) A compiler may issue a warning. However, I found that GCC 4.8.1 does not warn, even with -Wall and the definition of any_cast being known at the point of use.
You are absolutely right: ValueType&& any_cast(any&&) was not a good idea and it may break code that accepts result by const reference. Making it ValueType any_cast(any&&) would be more correct. I'll fix that as soon as the migration to the GIT will be finished. Great thanks for finding, investigating and reporting this issue! Created ticket #9462 : https://svn.boost.org/trac/boost/ticket/9462 -- Best regards, Antony Polukhin
2013/12/2 Antony Polukhin <antoshkka@gmail.com>
2013/12/2 Arne Vogel <avogel@benocs.com>
Hello Boost Users,
Code like the following used to work with Boost 1.53 on x86-64 Linux using GCC 4.7 and -std=c++11:
boost::any makeVec() { return std::vector<int>{ 1, 2, 3 }; }
void foo() { const auto& vec = boost::any_cast<std::vector<int>>(makeVec()); for ( auto value : vec ) { // ... } }
The same code is likely to cause a segmentation violation using Boost 1.55. According to the online documentation *1, any_cast has two overloads accepting reference arguments:
template<typename ValueType> ValueType any_cast(any&); template<typename ValueType> ValueType any_cast(const any&);
If these were indeed the only overloads accepting reference arguments, there is no reason why the example code shouldn't work. ValueType any_cast(const any&) should be the preferred overload and it returns a vector<int> temporary whose lifetime is then extended to the end of the block by being bound to a local const reference, as required by the C++ standard.
However, in any.hpp, I discovered the following overload, which is only declared if the macro BOOST_NO_CXX11_RVALUE_REFERENCES is not set. I suppose this overload was added since 1.53, or that, previously, it was by default omitted.
template<typename ValueType> ValueType&& any_cast(any&&);
This has higher precedence binding to the temporary returned from makeVec, and returns an rvalue reference to the vector<int> contained in said temporary, which is then implicitly converted to a const lvalue reference as by the usual language rules. Since the lifetime of the temporary returned by makeVec is NOT extended - this only works for temporaries directly bound to a local const reference, whereas the result of makeVec is "hidden" inside the call to any_cast - the temporary is destroyed before the start of the for loop, which renders vec invalid and the code a denizen of undefined behavior land.
So much for the analysis, now for my actual question. :-)
Why does the new overload return by reference when the old ones return by value? The rationale for the old overloads was, I believe, "you should have to ask for a reference if you want one", which makes sense given precisely that using references tends to be more error prone if temporaries are involved. So this would be the very reason why Any does not declare:
template<typename ValueType> const ValueType& any_cast(const any&);
even though it could be implemented as easily as the actual overload. If I really wanted to get an const reference out of any_cast, I could write something like
boost::any_cast<const ValueType&>(var);
I can't think of a reason to be inconsistent in this regard and return a reference if a temporary is passed to any_cast, instead of requiring a user who really wants any_cast to return an rvalue reference to write any_cast<ValueType&&>(...). That the overload breaks existing code and (due to being undocumented and not checked at compile time *2), silently so, whereas using ValueType any_cast(any&&) would at least fail to do any harm, is, in my opinion, another reason to be sceptical of the current solution.
Furthermore, there is a usability reason for preferring ValueType(any &&): If ValueType &&(any &&) is declared and I do NOT want a reference, I need to write more verbose and inelegant code than in the opposite case:
// Name the temporary const auto &vecAny = makeVec(); // Now, ValueType&& any_cast(any&&) is eliminated from candidate set (no conversion from an lvalue to an rvalue reference) const auto& vec = boost::any_cast<std::vector<int>>(vecAny);
Note that the const is accidental here, using a mutable lvalue would work as well.
Looking forward to your opinions, Arne Vogel
*1) http://www.boost.org/doc/libs/1_55_0/doc/html/boost/any_ cast_idp25247624.html *2) A compiler may issue a warning. However, I found that GCC 4.8.1 does not warn, even with -Wall and the definition of any_cast being known at the point of use.
You are absolutely right: ValueType&& any_cast(any&&) was not a good idea and it may break code that accepts result by const reference. Making it ValueType any_cast(any&&) would be more correct.
I'll fix that as soon as the migration to the GIT will be finished.
Great thanks for finding, investigating and reporting this issue!
Created ticket #9462 : https://svn.boost.org/trac/boost/ticket/9462
That said, what do you gain by doing: const auto& vec = boost::any_cast<std::vector<int>>(makeVec()); instead of const auto vec = boost::any_cast<std::vector<int>>(makeVec()); ? Regards, Kris
On 03.12.2013 00:01, Krzysztof Czainski wrote:
2013/12/2 Antony Polukhin <antoshkka@gmail.com <mailto:antoshkka@gmail.com>>
You are absolutely right: ValueType&& any_cast(any&&) was not a good idea and it may break code that accepts result by const reference. Making it ValueType any_cast(any&&) would be more correct.
I'll fix that as soon as the migration to the GIT will be finished.
Great thanks for finding, investigating and reporting this issue!
Created ticket #9462 : https://svn.boost.org/trac/boost/ticket/9462
Thanks a lot!
That said, what do you gain by doing: const auto& vec = boost::any_cast<std::vector<int>>(makeVec()); instead of const auto vec = boost::any_cast<std::vector<int>>(makeVec()); ?
Regards, Kris
In C++11 with a moveable type, probably not much. I don't quite remember why I wrote the code like this - it could have been out of habit since in C++98 using a reference may save a copy, though I guess an optimizing compiler had already been allowed to elide it. Regards, Arne
2013/12/4 Arne Vogel <avogel@benocs.com>
On 03.12.2013 00:01, Krzysztof Czainski wrote: [...] what do you gain by doing:
const auto& vec = boost::any_cast<std::vector<int>>(makeVec()); instead of const auto vec = boost::any_cast<std::vector<int>>(makeVec()); ?
Regards, Kris
In C++11 with a moveable type, probably not much. I don't quite remember why I wrote the code like this - it could have been out of habit since in C++98 using a reference may save a copy, though I guess an optimizing compiler had already been allowed to elide it.
I'm just curious to know if anyone uses a compiler that doesn't actually elide the copy (or move) in this case ;-) In all discussions about this I've never seen someone actually name one, but still they try to avoid the copy just in case. I think taking values returned by value by const & to extend their lifetime should be deprecated in new C++ - compilers could then warn not to do it. Cheers, Kris
On Wed, Dec 4, 2013 at 7:07 AM, Krzysztof Czainski <1czajnik@gmail.com>wrote: I think taking values returned by value by const & to extend their lifetime
should be deprecated in new C++ - compilers could then warn not to do it.
I like that idea! One C++03 usage that has bothered me for a long time is a base class which stores one or more std::string data members, with virtual methods that return const std::string&. In an override method on one existing subclass, I decide to compute a std::string return value. But I can't return a temporary, and I can't return a stack variable! I have two bad choices: I can add a new private std::string data member to the subclass specifically to store the return value from this method; or I can refactor every declaration of this method in the entire class hierarchy to return by value. (We will loftily ignore the even worse choice of storing a module-static std::string for the method return value.) I have several times decided to refactor the method signature throughout the class hierarchy. But by the time I'm doing that much work, I typically also change the signature of every virtual method returning const std::string& to return by value -- to spare myself, or someone else, the pain of having to do it later. I've pretty much concluded that a virtual method returning const& is an anti-pattern. I like the idea of a compiler warning about it.
On 12/4/2013 7:07 AM, Krzysztof Czainski wrote:
2013/12/4 Arne Vogel <avogel@benocs.com>
On 03.12.2013 00:01, Krzysztof Czainski wrote: [...] what do you gain by doing:
const auto& vec = boost::any_cast<std::vector<int>>(makeVec()); instead of const auto vec = boost::any_cast<std::vector<int>>(makeVec()); ?
Regards, Kris
In C++11 with a moveable type, probably not much. I don't quite remember why I wrote the code like this - it could have been out of habit since in C++98 using a reference may save a copy, though I guess an optimizing compiler had already been allowed to elide it.
I'm just curious to know if anyone uses a compiler that doesn't actually elide the copy (or move) in this case ;-) In all discussions about this I've never seen someone actually name one, but still they try to avoid the copy just in case.
I think taking values returned by value by const & to extend their lifetime should be deprecated in new C++ - compilers could then warn not to do it.
Cheers, Kris
An old article comes to mind which uses exactly this idea to implement a little API to help people write exception-safe code [1]. This is part of the Loki library [2]. I think this is still considered a legitimate technique, right? Andy 1. http://www.drdobbs.com/cpp/generic-change-the-way-you-write-excepti/18440375... 2. http://loki-lib.sourceforge.net/
participants (2)
-
Antony Polukhin -
Arne Vogel -
Krzysztof Czainski -
Michael Chisholm -
Nat Goodspeed