Is the following code causing undefined behavior?
std::map<int, vector<int>> foo()
{
return ...
}
BOOST_FOREACH(const int& i, foo()[42])
{
std::cout << i << std::endl;
}
If undefined, What is the good way to fix it? What if I use c++11 range-for loop instead of BOOST_FOREACH?
This is, unfortunately, most probably undefined behavior.
The problem is that you have two levels here:
std::map<...> is an r-value, its lifetime will be expanded until the end of the full-expressionstd::vector<int>& is an l-value reference (into an object), its lifetime is that of the object.The problem arises because the code (roughly) expands to something like:
// from
for (<init>: <expr>) {
<body>
}
// to
auto&& __container = <expr>;
for (auto __it = begin(container), __e = end(container); __it != __e; ++__it)
{
<init> = *__it;
<body>
}
The issue here is in the initialization of __container:
auto&& __container = foo()[42];
If it where just foo(), this would work, because the lifetime of std::map<...> would be extended to match that of __container, however in this case we get:
// non-standard gcc extension, very handy to model temporaries:
std::vector<int>& __container = { std::map<...> m = foo(); m[42] };
And thus __container ends up pointing into the nether.
The return value exists until the end of the full expression
which creates it. So it all depends on how BOOST_FOREACH
expands; if it creates a scope outside of the for loop, and
copies the return value to a variable in it (or uses it to
initialize a reference), then you're safe. If it doesn't,
you're not.
The C++11 range-for loop basically has the semantics of binding to a reference in a scope outside of a classic for-loop, so it should be safe.
EDIT:
This would apply if you were capturing the return value of
foo. As Benjamin Lindley points out, you aren't. You're
capturing the return value of operator[] on a map. And this
is not a temporary; it is a reference. So no extension of
lifetime occurs, neither in BOOST_FOREACH nor in range-for.
Which means that the map itself will be destructed at the end of
the full expression which contains the function call, and that
undefined behavior occurs. (Boost could, I suppose, make a deep
copy of the map, so you'd be safe. But somehow, I doubt that it
does.)
END OF EDIT:
Never the less, I would question the wisdom of returning an
std::map when all you want is a single entry in it. If the
map actually exists outside the function (is not on the heap),
then I'd return a reference to it. Otherwise, I'd find some
what that it did.
From: http://www.boost.org/doc/libs/1_55_0/doc/html/foreach.html
Iterate over an expression that returns a sequence by value (i.e. an rvalue):
extern std::vector<float> get_vector_float();
BOOST_FOREACH( float f, get_vector_float() )
{
// Note: get_vector_float() will be called exactly once
}
So it is well defined and works.
As well, it is well defined in C++11 (and works):
for (const int& i : get_vector()) // get_vector() computed only once
{
std::cout << i << std::endl;
}
The problem here is that foo()[42] returns a reference from a temporary (via a method).
auto& v = foo()[42];
life of foo() temporary is not extended...
You may solve that by extending foo temporary lifetime
auto&& m = foo();
for (const int& i : m[42]) {
std::cout << i << std::endl;
}
