Re: Nasty bug in Boost 1.32.0 bind]

Sean Parent wrote:
On Dec 6, 2004, at 12:10 PM, Peter Dimov wrote:
This is not a bug. Bind was deliberately changed to return by value because of a bug report similar to the following:
pair_t make_pair( int, int );
bind( &pair_t::first, bind( make_pair, 10, 20 ) )();
The inner bind returns by value (because make_pair returns a value) and the outer bind returns a reference to a temporary.
A similar, but rare, situation can arise when bind is invoked on an rvalue:
pair_t const make_pair();
bind( &pair_t::first, _1 )( make_pair() );
I'm more interested in the actual code that was broken by this change, because the assert() above was never required to pass. By-value or by-reference was (and probably still is) an open question. Of course I should have gotten it right in the first place.
The documentation explicitly states that it will return a const reference: -----
bind(&X::f, args)
is equivalent to
bind<R>(mem_fn(&X::f), args)
where R is the return type of X::f (for member functions) or a const reference to the type of the member (for data members.)
Yes, you are right. I missed that.
The actual failure was in conjunction with boost::function() where it fails silently because of a cast to result type inside boost function. Here is a snippet that was my first attempt at isolating the problem:
---- #include <boost/bind.hpp> #include <boost/function.hpp> #include <utility> #include <cassert>
int main() { typedef std::pair<int, int> pair_t;
pair_t pair(10, 20);
boost::function<const int&(const pair_t&)> function(boost::bind(&pair_t::first, _1));
const int& x = function(pair);
assert(&pair.first == &x);
return 0; }
I get warning C4172 from VC++ 7.1, "returning address of local variable or temporary", on this example (in function_template.hpp:111). The example can be further simplified to: #include <boost/function.hpp> int f() { return 5; } int main() { boost::function<int const & ()> g( f ); int const & r = g(); } which produces the same warning. A static_assert in boost::function would probably help on compilers that don't emit a warning. [...]
The explicit version:
const int& x = boost::bind<int&>(&pair_t::first, _1)(pair);
should work on conforming compilers.
Good to know - I will review my code and try to be explicit where possible - however, I use bind() a lot within templates and rely on it not making an additional (often expensive) copy of my members. I don't think there there is a correct answer for the default behavior (between returning a const& and a value) but the docs need to be consistent - and a change of this nature needs to be called out in the release notes.
Yes, you are right. I underestimated the impact of this change; the new behavior was supposed to eliminate this sort of bug, not introduce it. Breaking existing code is never pleasant. The change has been in the CVS for some time, but unfortunately most real-world testing occurs on the actual releases.

On Dec 6, 2004, at 2:02 PM, Peter Dimov wrote:
I get warning C4172 from VC++ 7.1, "returning address of local variable or temporary", on this example (in function_template.hpp:111).
In CodeWarrior 9.3 BOOST_NO_VOID_RETURNS is not defined so in function.hpp the code falls into the static_cast<> case ----- # ifndef BOOST_NO_VOID_RETURNS return static_cast<result_type>(result); # else return result; # endif // BOOST_NO_VOID_RETURNS ------ This silences the warning - and you get no indication that anything is wrong. Just to make sure - you plan to leave it returning by value? If that is the case I'll update my code (I had patched my copy of boost). Thanks! Sean

Sean Parent wrote:
On Dec 6, 2004, at 2:02 PM, Peter Dimov wrote:
I get warning C4172 from VC++ 7.1, "returning address of local variable or temporary", on this example (in function_template.hpp:111).
In CodeWarrior 9.3 BOOST_NO_VOID_RETURNS is not defined so in function.hpp the code falls into the static_cast<> case
-----
# ifndef BOOST_NO_VOID_RETURNS return static_cast<result_type>(result); # else return result; # endif // BOOST_NO_VOID_RETURNS
[Thanks for posting to the list, Sean -- this is a Boost.Function question so you should indicate it in the subject line] ?? That doesn't look like a very good way to be using BOOST_NO_VOID_RETURNS. What's going on here? -- Dave Abrahams Boost Consulting http://www.boost-consulting.com

On Dec 6, 2004, at 6:03 PM, David Abrahams wrote:
Sean Parent wrote:
On Dec 6, 2004, at 2:02 PM, Peter Dimov wrote:
I get warning C4172 from VC++ 7.1, "returning address of local variable or temporary", on this example (in function_template.hpp:111).
In CodeWarrior 9.3 BOOST_NO_VOID_RETURNS is not defined so in function.hpp the code falls into the static_cast<> case ----- # ifndef BOOST_NO_VOID_RETURNS return static_cast<result_type>(result); # else return result; # endif // BOOST_NO_VOID_RETURNS
[Thanks for posting to the list, Sean -- this is a Boost.Function question so you should indicate it in the subject line]
?? That doesn't look like a very good way to be using BOOST_NO_VOID_RETURNS. What's going on here?
The static_cast<result_type> ensures that when result_type == void, the "return" doesn't need to disappear. It's essentially a workaround to a workaround for BOOST_NO_VOID_RETURNS. Anyway, it should be fixed now. Doug

Sean Parent wrote:
On Dec 6, 2004, at 2:02 PM, Peter Dimov wrote:
I get warning C4172 from VC++ 7.1, "returning address of local variable or temporary", on this example (in function_template.hpp:111).
In CodeWarrior 9.3 BOOST_NO_VOID_RETURNS is not defined so in function.hpp the code falls into the static_cast<> case
-----
# ifndef BOOST_NO_VOID_RETURNS return static_cast<result_type>(result); # else return result; # endif // BOOST_NO_VOID_RETURNS
------
This silences the warning - and you get no indication that anything is wrong.
Not good. :-) FWIW, VC++ 7.0+ do not define BOOST_NO_VOID_RETURNS either.
Just to make sure - you plan to leave it returning by value? If that is the case I'll update my code (I had patched my copy of boost).
Yes, return by value still seems to be the lesser of the two evils, since we can deal with the problematic case on boost::function's side, if Doug doesn't mind (the problem there is not bind-specific).
participants (4)
-
David Abrahams
-
Doug Gregor
-
Peter Dimov
-
Sean Parent