Range-based for loop temporaries lifetime

[Filip Konvička]

Hi,

I'm bringing this subject here from the boost.users newsgroup. The
original post was dealing with the following code.

#include <boost/range/adaptor/reversed.hpp>
#include <boost/foreach.hpp>
#include <vector>
#include <iostream>

std::vector<int> getv() {
return std::vector<int>{1,2,3};
}

int main(int argc, char* argv[]) {
// works fine:
BOOST_REVERSE_FOREACH(int i, getv())
std::cout << i << std::endl;
// crashes:
for(int i : boost::adaptors::reverse(getv()))
std::cout << i << std::endl;
return 0;
}

It turns out that the reason for the latter loop to crash is that the
temporary vector returned by getv() gets destroyed too soon, leaving
invalid iterators in the range resulting from 'reverse'. Clearly this
is because of how the range-based for loop is defined/designed.

I find the behavior counter-intuitive. IMHO the language syntax hides
the fact that there is a "hidden semicolon" in the for loop
implementation that causes the temporary to be destroyed soon. I wonder
whether this was an intention for the 'for' loop to behave like this,
and whether there are any advantages of that the temporaries like this
get destroyed so soon.

There is a related proposal by Eric Niebler [
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4128.html ], I
think that in particular sections 3.3.2 and 14.1 are relevant to my
question. However, I'm not interested in just ranges but rather
temporaries in general.

Thanks,
Filip

[Ville Voutilainen]

For what it's worth, this is
http://cplusplus.github.io/EWG/ewg-active.html#120

[Filip Konvička]

Dne 26. 11. 2014 9:35, Ville Voutilainen napsal(a):

Thanks. So it is being discussed even now, which is good. I'm not sure
where else than here to post my concerns, so I'll just say here that
this syntactic sugar we got with range-based for is unfortunately
sweet-and-sour :) With the current state of things I'm scared to death
to let anyone in my group, myself included, use this.

Anyway, still, thanks everyone for your work on the standard. And three
times hooray to the Valgrind team who will save us all in the end.

Cheers,
Filip

[David Krauss]

On 2014–11–26, at 4:11 PM, Filip Konvička <filip.k...@logis.cz> wrote:

There is a related proposal by Eric Niebler [ http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4128.html ], I think that in particular sections 3.3.2 and 14.1 are relevant to my question.  However, I'm not interested in just ranges but rather temporaries in general.

This issue was addressed quite directly by N4221. The proposal was received pretty positively, except that they wanted to see it extended to lambdas and std::function. So the committee is not interested in fixing this:

for(int i : boost::adaptors::reverse(getv()))
   std::cout << i << std::endl;

without also fixing this:

for(int i : [] { return boost::adaptors::reverse(getv()); }() )

   std::cout << i << std::endl;

What I failed to mention in the presentation was that the proposal gives implementations the power to warn on the latter case, because the lambda return value is holding a reference to a temporary. The idea is only mentioned briefly in the examples section, at the bottom of page 9 of N4221.

I didn’t connect the problem to the solution at the time, but such a warning can probably cover the vast majority of such generic-abuse cases. Perhaps we haven’t heard the last of this solution, yet. (Also, no other solutions have been forthcoming, but two people essentially converged to lifetime extension through decorated function parameters.)

[Filip Konvička]

Dne 26. 11. 2014 12:21, David Krauss napsal(a):

>
> On 2014–11–26, at 4:11 PM, Filip Konvička <filip.k...@logis.cz

> <mailto:filip.k...@logis.cz>> wrote:
>
>> There is a related proposal by Eric Niebler
>> [http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4128.html],
>> I think that in particular sections 3.3.2 and 14.1 are relevant to my
>> question. However, I'm not interested in just ranges but rather
>> temporaries in general.
>
> This issue was addressed quite directly by N4221

> <http://open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4221.pdf>. The

> proposal was received pretty positively, except that they wanted to see
> it extended to lambdas and std::function. So the committee is not
> interested in fixing this:
>
> for(int i : boost::adaptors::reverse(getv()))
> std::cout << i << std::endl;
>
> without also fixing this:
>
> for(int i : [] { return boost::adaptors::reverse(getv()); }() )
> std::cout << i << std::endl;
>
> What I failed to mention in the presentation was that the proposal gives
> implementations the power to warn on the latter case, because the lambda
> return value is holding a reference to a temporary. The idea is only
> mentioned briefly in the examples section, at the bottom of page 9 of N4221.
>
> I didn’t connect the problem to the solution at the time, but such a
> warning can probably cover the vast majority of such generic-abuse
> cases. Perhaps we haven’t heard the last of this solution, yet. (Also,
> no other solutions have been forthcoming, but two people essentially
> converged to lifetime extension through decorated function parameters.)

Thanks, that looks like it could address the issue in a more general way
(I'm not trying to imply that I have understood all of the proposal).

So my conclusion is that nobody really likes the way this currently
works and there are people actively working on enhancing the standard.
Thank you.

Filip

[Thiago Macieira]

On Wednesday 26 November 2014 19:21:55 David Krauss wrote:
> without also fixing this:
>
> for(int i : [] { return boost::adaptors::reverse(getv()); }() )
> std::cout << i << std::endl;
>
> What I failed to mention in the presentation was that the proposal gives
> implementations the power to warn on the latter case, because the lambda
> return value is holding a reference to a temporary. The idea is only
> mentioned briefly in the examples section, at the bottom of page 9 of
> N4221.

Is there any proposal on how to accomplish the warning or even the fixing in
this case?

Because it seems that we'd give power to the compiler to warn or forbid that
in:

auto foo() { return some_wrapper(getv()); }

the returned type (whichever it is!) contains a dangling reference.
--
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
Software Architect - Intel Open Source Technology Center
PGP/GPG: 0x6EF45358; fingerprint:
E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358

[Olaf van der Spek]

Op woensdag 26 november 2014 09:30:09 UTC+1 schreef Filip Konvička:

   // works fine:
   BOOST_REVERSE_FOREACH(int i, getv())
     std::cout << i << std::endl;
   // crashes:
   for(int i : boost::adaptors::reverse(getv()))
     std::cout << i << std::endl;
   return 0;
}

Wouldn't BOOST_FOREACH(int i, boost::adaptors::reverse(getv())) also crash?

What if I assign the result of reverse() to a var? A solution for for wouldn't cover it would it?

[Filip Konvička]

Dne 26. 11. 2014 20:55, Olaf van der Spek napsal(a):

It does not crash; I think that it is capturing the iterable in an 'if'
declaration, something like

if ( auto const& my_range = boost::adaptors::reverse(getv()) )
... (rest of the loop implementation) ...

It's more complicated than this but the point is (I think) that all this
is just one statement. Range-based for, in contrast, is two (internally).

You could assign the results of the "getv()" call to a variable to
resolve this.

auto const& temp = getv();
for(int i : boost::adaptors::reverse(temp))
...

...would work just fine. But it's so unobvious why the original code
wouldn't work when this does - which is why everyone is trying to come
up with a solution that would actually make this work.

[Olaf van der Spek]

On Wed, Nov 26, 2014 at 9:31 PM, Filip Konvička <filip.k...@logis.cz> wrote:
> It does not crash; I think that it is capturing the iterable in an 'if'
> declaration, something like
>
> if ( auto const& my_range = boost::adaptors::reverse(getv()) )
> ... (rest of the loop implementation) ...
>
> It's more complicated than this but the point is (I think) that all this is
> just one statement. Range-based for, in contrast, is two (internally).

Ah, that's smart.
But the general problem still exists:
auto& t = boost::adaptors::reverse(getv());
// use t (and crash)

Extending lifetimes of the for expression is still a good idea though.

--
Olaf

[Filip Konvička]

> This issue was addressed quite directly by N4221

> <http://open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4221.pdf>. The

> proposal was received pretty positively, except that they wanted to see
> it extended to lambdas and std::function. So the committee is not
> interested in fixing this:
>
> for(int i : boost::adaptors::reverse(getv()))
> std::cout << i << std::endl;
>
> without also fixing this:
>
> for(int i : [] { return boost::adaptors::reverse(getv()); }() )
> std::cout << i << std::endl;

The other post made me try this for fun (obviously in C++14 mode):

[](auto r) {
for(auto i : r)

std::cout << i << std::endl;

}(boost::adaptors::reverse(getv()));

It works for the first case, but unfortunately not for the second with
the lambda. I wonder, is the for loop even relevant to the lambda
problem? The code

[]{ return boost::adaptors::reverse(getv()); }()

already seems errorneous, doesn't it? My understanding is that you're
trying to issue a warning about the fact that the range being returned
outlives the referenced iterable. Or is the code supposed to be valid?

[Filip Konvička]

Dne 26. 11. 2014 21:55, Olaf van der Spek napsal(a):

Oh, by the way, I was wrong, it does crash. I haven't noticed that you
replaced the BOOST_REVERSE_FOREACH with BOOST_FOREACH (+ the reversing
adapter).

The smart ideas in BOOST_FOREACH are Eric Niebler's. There is an
article on the making of the BOOST_FOREACH macro that is worth reading.

http://www.artima.com/cppsource/foreach.html

[David Krauss]

On 2014–11–27, at 2:58 AM, Thiago Macieira <thi...@macieira.org> wrote:

Is there any proposal on how to accomplish the warning or even the fixing in 
this case?

Because it seems that we'd give power to the compiler to warn or forbid that 
in:

auto foo() { return some_wrapper(getv()); }

the returned type (whichever it is!) contains a dangling reference.

Yeah, the return statement having a value which is lifetime-associated (proposed term) with a temporary would generate a warning (or error), whether it’s inside a lambda or a normal function.

Besides return values, other cases of lifetime-association between a temporary and a persistent object that does not grant lifetime extension should elicit warnings. The proposal uses a new-expression for illustration.

On 2014–11–27, at 5:13 AM, Filip Konvička <filip.k...@logis.cz> wrote:

 []{ return boost::adaptors::reverse(getv()); }()

already seems errorneous, doesn't it? My understanding is that you're trying to issue a warning about the fact that the range being returned outlives the referenced iterable.  Or is the code supposed to be valid?

Yes, the compiler would flag that, regardless of context (loop or whatever).

[Thiago Macieira]

On Thursday 27 November 2014 07:17:07 David Krauss wrote:
> On 2014–11–27, at 2:58 AM, Thiago Macieira <thi...@macieira.org> wrote:
> > Is there any proposal on how to accomplish the warning or even the fixing
> > in this case?
> >
> >
> >
> > Because it seems that we'd give power to the compiler to warn or forbid
> > that >
> > in:
> >
> >
> > auto foo() { return some_wrapper(getv()); }
> >
> >
> >
> > the returned type (whichever it is!) contains a dangling reference.
>
> Yeah, the return statement having a value which is lifetime-associated
> (proposed term) with a temporary would generate a warning (or error),
> whether it’s inside a lambda or a normal function.
>
> Besides return values, other cases of lifetime-association between a
> temporary and a persistent object that does not grant lifetime extension
> should elicit warnings. The proposal uses a new-expression for
> illustration.

I understand a good compiler warning about it, provided everything is inline.

But I don't see how this could be fixed, even for lambdas.

[David Krauss]

On 2014–11–27, at 8:00 AM, Thiago Macieira <thi...@macieira.org> wrote:

I understand a good compiler warning about it, provided everything is inline.

Again, the return statement is what is defective. It doesn’t matter if the function is inline or if it’s even called.

But I don't see how this could be fixed, even for lambdas.

I don’t think we even want a solution that potentially extends the lifetime of a function-local variable into the scope of its call site.

For lambda parameters, the fix is to decorate them as you would any other parameter. Lambda init-captures are lifetime-associated with the resulting closure object. I don’t think there’s a use-case for decorating the lambda operator() implicit this parameter, but the syntax is there if we want it. To fix Filip’s  last example:

 [](export auto r) {
   for(auto i : r)

     std::cout << i << std::endl;

 }(boost::adaptors::reverse(getv()));

For indirect calls, such as std::function and function pointers, any fix requires modifying the type system (and worse, introspecting into overload selection), or determining parameter object lifetime at runtime, which is right out. Diagnosis, as opposed to fixing, is simple: std::function incurs the aforementioned diagnosis in its call wrapper return statement as it does not decorate anything, and the compiler can see decoration mismatches in function pointer binding and virtual overrides.

The critical issue is what level of diagnosis should happen. I proposed a very lax rule, but it was unpopular at the meeting. More eager, stricter, and completely-specified diagnosis rules could make the proposal stronger.

[Nathan Ernst]

My guess as to why the latter original fails is because of an implementation defect of your compiler/stdlib. I don't see why, substituting your calls into the equivalent code shown in 6.5.4, it would fail. I initially suspected that your temporary expression was being evaluated multiple times (and it may be according to your described behavior), but it shouldn't be (given that range-init expression is pretty loosely defined...).

To me, this seems like a compiler defect, not a language defect, as presented.

For reference, the prescribed equivalency:

{

  auto && __range = range-init;

    for ( auto __begin = begin-expr,

      __end = end-expr;

      __begin != __end;

      ++__begin ) {

    for-range-declaration = *__begin;

   statement

  }

}

--

---
You received this message because you are subscribed to the Google Groups "ISO C++ Standard - Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to std-discussio...@isocpp.org.
To post to this group, send email to std-dis...@isocpp.org.
Visit this group at http://groups.google.com/a/isocpp.org/group/std-discussion/.

[David Krauss]

On 2014–11–27, at 8:52 AM, David Krauss <pot...@gmail.com> wrote:

To fix Filip’s  last example:

 [](export auto r) {
   for(auto i : r)
     std::cout << i << std::endl;
 }(boost::adaptors::reverse(getv()));

Woops, that’s not right. There’s no lifetime extension needed there. I was going for

for(int i : [] ( export auto r ) { return boost::adaptors::reverse(r); }(getv()) )

[Thiago Macieira]

On Wednesday 26 November 2014 18:54:20 Nathan Ernst wrote:
> To me, this seems like a compiler defect, not a language defect, as
> presented.
>
> For reference, the prescribed equivalency:
>
> {
> auto && __range = range-init;
> for ( auto __begin = begin-expr,
> __end = end-expr;
> __begin != __end;
> ++__begin ) {
> for-range-declaration = *__begin;
> statement
> }
> }

And that's exactly the problem. There's a sequence point after the range-init.
So while there the result of the initialisation is lifetime-extended to the
end of the block, the side-effects of range-init aren't.