Boost C++ Libraries: Ticket #5475: [Foreach] rvalue reference binding of temporary ranges

mimomorin@… — Fri, 15 Apr 2011 03:38:00 GMT

I will attach patches for boost/foreach.hpp and libs/foreach/test/.

attachment set

mimomorin@… — Fri, 15 Apr 2011 05:37:11 GMT

attachment → test_noncopyable_rvalue.patch

A patch for libs/foreach/test (against trunk) to test iteration over non-copyable and non-movable rvalue ranges.

mimomorin@… — Fri, 15 Apr 2011 06:11:04 GMT

Hmm, I cannot attach a patch for foreach.hpp...

The patch I made does the following:

(Note: contrary to C++0x range-based for, this does not allow mutable iteration over rvalue ranges; Non-const rvalue ranges are bounded to const rvalue references.)

Adding a configuration macro

BOOST_FOREACH_USE_RVALUE_REFERENCE_BINDING
- When a compiler supports rvalue references and decltype, this macro is defined and rvalue reference binding is used.

Adding macros for auto declarations

BOOST_FOREACH_AUTO_OBJECT(NAME, EXPR)
- Equivalent to auto NAME = EXPR;.
BOOST_FOREACH_AUTO_REF_REF(NAME, EXPR)
- Equivalent to auto && NAME = EXPR;if EXPR is an lvalue.
- Equivalent to auto const && NAME = EXPR;if EXPR is an rvalue.

Adding overloaded functions

begin
end
- For C-strings, this function returns cstr_end_iterator, which is defined as an empty struct. I also overloaded operator != to support C-strings:
```
template <typename Iterator>
inline bool operator !=(Iterator cur, cstr_end_iterator)
{
    return *cur != 0;
}
```
rbegin
rend

All the regression tests ran successfully with the following compilers (both in C++03 and C++0x modes): gcc-4.3.5, gcc-4.4.5, gcc-4.5.2, gcc-4.6.0, clang (TOT).

Newly added tests (in test_noncopyable_rvalue.patch) ran successfully in a C++0x mode.

attachment set

Fri, 15 Apr 2011 07:47:13 GMT

attachment → foreach_cxx0x_v2.patch

A patch for foreach.hpp (against trunk) to bind temporary ranges to const rvalue references.

Fri, 15 Apr 2011 08:06:23 GMT

All right, succeeded to attach patches!

status changed

Eric Niebler — Tue, 20 Sep 2011 02:14:31 GMT

status new → assigned

On MSVC 9 and 10, I get:

noncopyable_rvalue_const.cpp(60) : error C2248: 'my_container::my_container' : cannot access private member declared in class 'my_container'
        noncopyable_rvalue_const.cpp(43) : see declaration of 'my_container::my_container'
        noncopyable_rvalue_const.cpp(19) : see declaration of 'my_container'

I can't apply the patch until these issues are resolved. Any ideas?

Tue, 20 Sep 2011 05:52:33 GMT

Thank you for seeing this.

On MSVC 9, the following error should emit…

#ifndef BOOST_FOREACH_USE_RVALUE_REFERENCE_BINDING
# error Expected failure : non-copyable rvalues disallowed
#else

So, the error message you posted is for MSVC 10?

Eric Niebler — Tue, 20 Sep 2011 13:42:31 GMT

Ah, yes! I was mistakenly using vc10 even in my vc9 environment. I get the proper error with vc9. The above error is with vc10.

Tue, 20 Sep 2011 15:39:08 GMT

OK :)

Well, I don't have access to MSVC 10, so could you compile the following codes and tell me what compiler error happens?

A.cpp

class my_container
{
public:
    my_container()
    {}
private:
    int array_[5];
    my_container(my_container const &);
    my_container &operator =(my_container const &);
    my_container(my_container &&);
    my_container &operator =(my_container &&);
};
typedef my_container const const_my_container;
int main (int argc, char* argv[])
{
    my_container const && x = const_my_container();
    return 0;
}

B.cpp

class my_container
{
public:
    my_container()
    {}
// private:
    int array_[5];
    my_container(my_container const &);
    my_container &operator =(my_container const &);
    my_container(my_container &&);
    my_container &operator =(my_container &&);
};
typedef my_container const const_my_container;
int main (int argc, char* argv[])
{
    my_container const && x = const_my_container();
    return 0;
}

Thanks in advance.

Eric Niebler — Tue, 20 Sep 2011 18:39:27 GMT

A.cpp

1>c:\boost\org\trunk\libs\proto\scratch\main.cpp(21): error C2248: 'my_container::my_container' : cannot access private member declared in class 'my_container'
1>          c:\boost\org\trunk\libs\proto\scratch\main.cpp(10) : see declaration of 'my_container::my_container'
1>          c:\boost\org\trunk\libs\proto\scratch\main.cpp(2) : see declaration of 'my_container'

B.cpp No error

Eric Niebler — Tue, 20 Sep 2011 18:41:41 GMT

P.S. If you have Windows, you have access to MSVC 10: @http://www.microsoft.com/visualstudio/en-us/products/2010-editions/visual-cpp-express

Wed, 21 Sep 2011 04:12:04 GMT

Thanks for doing this.

Considering the results, I think the error happens due to the incomplete implementation of C++11 (i.e. DR 391) in MSVC 10. The code should compile fine in C++11.

The fix for boost/foreach.hpp could be

Do nothing (BOOST_FOREACH fails with noncopyable rvalues on MSVC 10).
Revert to the C++03 BOOST_FOREACH on MSVC 10.
Write a special code for MSVC 10.

I prefer A, since this does not break anything (noncopyable rvalues cannot be handled by the C++03 BOOST_FOREACH, IIUC).

For libs/foreach/test/noncopyable_rvalue_***.hpp, I can change them to pass the tests on MSVC 10 by declaring copy/move ctors and copy/move assignment operators public. But, wouldn't it be better to fail the tests on MSVC 10 to explicitly state that noncopyable rvalues are not supported?

P.S. Will try to setup Visual Studio 2010 Express. Thanks!

Eric Niebler — Thu, 22 Sep 2011 15:36:03 GMT

(A) would be fine, I think. We can mark up the expected failures specially. But if you think this is a bug in MSVC, you *must* file a bug here: <https://connect.microsoft.com/VisualStudio/>. (You may need to open an account there to do so.) Use the small repro case you sent me and refer to the relevant part of the standard. Also, double-check that Comeau and GCC accept the code in 0x mode. You can test with Comeau here: <http://www.comeaucomputing.com/tryitout/>. Let me know if you have any problems. Let's get this fixed.

Eric Niebler — Thu, 22 Sep 2011 18:02:30 GMT

I considered just applying this change, msvc10 be damned, but then I realized that I don't know enough about C++11 to say whether the code is well-formed, and Comeau Online rejected your A.cpp source above. Comeau C++11 support is far from perfect, but it makes me uneasy that both msvc and Comeau reject the code. Can you quote the relevant parts of the standard that make A.cpp well-formed?

Thu, 22 Sep 2011 18:19:30 GMT

I tried Comeau Online and got compilation errors. But it seems that Comeau doesn't recognize rvalue references...

The relevant part is 8.5.3 p5:

If the initializer expression

is an xvalue, class prvalue, array prvalue or function lvalue and “cv1 T1” is reference-compatible with “cv2 T2”, or
has a class type (i.e., T2 is a class type), where T1 is not reference-related to T2, and can be implicitly converted to an xvalue, class prvalue, or function lvalue of type “cv3 T3”, where “cv1 T1” is reference-compatible with “cv3 T3”,

then the reference is bound to the value of the initializer expression in the first case and to the result of the conversion in the second case (or, in either case, to an appropriate base class subobject). In the second case, if the reference is an rvalue reference and the second standard conversion sequence of the user-defined conversion sequence includes an lvalue-to-rvalue conversion, the program is ill-formed.

See also links in this answer http://stackoverflow.com/questions/1827049/

Thu, 22 Sep 2011 18:39:52 GMT

The problem in MSVC is that it does the access check of the copy constructor when binding references to rvalues even when the check should not occur. (In C++03, the check is legal.)

Here is a minimal code that demonstrates this.

class noncopyable
{
public:
    noncopyable()
    {}
private:
    noncopyable(noncopyable const &);
};
int main (int argc, char* argv[])
{
    noncopyable const& x = noncopyable();
    return 0;
}

This code fails to compile on MSVC, but compiles fine on Comeau.

Thu, 22 Sep 2011 19:03:28 GMT

Replying to eric_niebler:

But if you think this is a bug in MSVC, you *must* file a bug here: <https://connect.microsoft.com/VisualStudio/>.

Done! https://connect.microsoft.com/VisualStudio/feedback/details/690244/

Thu, 22 Sep 2011 19:31:26 GMT

Replying to Michel Morin <mimomorin@…>:

See also links in this answer http://stackoverflow.com/questions/1827049/

Specifically, I meant this link http://gcc.gnu.org/bugs/#cxx%5Frvalbind

Eric Niebler — Mon, 26 Sep 2011 18:17:10 GMT

Committed to trunk in [74584].

status changed; resolution set

Eric Niebler — Tue, 27 Sep 2011 16:31:33 GMT

status assigned → closed
resolution → fixed

(In [74591]) merge [74584] from trunk, fixes #5475

Wed, 05 Oct 2011 01:45:34 GMT

Thanks for resolving this ticket, Eric!

P.S. The bug reported to Microsoft Connect has been resolved saying "this issue has been fixed. The fix should show up in a future release of Visual C++."

Eric Niebler — Wed, 05 Oct 2011 01:47:44 GMT

Very glad to hear it. And thanks for your contribution to Boost!

Eric Niebler — Sat, 15 Oct 2011 19:24:42 GMT

There seems to be a problem with this on gcc-4.4.3. See, for instance, this. Michel, can you please look into this soon? We're about to lock down the release branch for 1.48, and I hate to regress Boost.Foreach.

Sun, 16 Oct 2011 02:57:34 GMT

I'm investigating now.

Sun, 16 Oct 2011 07:09:39 GMT

The current implementation of C++11 BOOST_FOREACH is based on decltype. However, gcc-4.4 has a bug that prevents us from using decltype in BOOST_FOREACH. (The bug has been fixed in gcc-4.5 and newer versions.)

The reason I used decltype rather than auto is that

auto && is buggy in gcc-4.4.*, 4.5.*, 4.6.0 and so we should not use it. (This bug is already fixed in gcc-4.6.1 and 4.7.)
decltype is more widely available than auto.
binding rvalue ranges to const references can be easily implemented with decltype than with auto.

Short-term fixed would be:

Revert to C++03 BOOST_FOREACH on gcc-4.4
Disable const reference biding of rvalue ranges on gcc-4.4. That is, rvalue ranges are bound to const or non-const rvalue ranges using auto &&. In spite of the above bug, auto && can be safely used in gcc-4.4, since lvalues are allowed to be bound to rvalue references in gcc-4.4.

A and B are extremely easy to do without causing any error.

Long-term fixed would be

Implement "const reference binding of rvalue ranges" with auto && on gcc-4.4. I already implemented it in this post.

Comments? I'm ready to make patches.

attachment set

Sun, 16 Oct 2011 12:13:29 GMT

attachment → fix_gcc_4_4_regression_by_C.patch

A patch for fixing gcc-4.4 regression (against trunk). This patch implements C of comment 23.

Sun, 16 Oct 2011 12:20:54 GMT

Attached a patch that implements C of comment 23. This patch should only affect a C++0x mode of gcc-4.4.

I ported the code in this post. But, due to auto&& bug in gcc-4.4, I needed to change the code for adding const qualification to rvalue ranges. With this patch, BOOST_FOREACH(DECL, EXPR) does the following in a C++0x mode of gcc-4.4:

Expand EXPR's lifetime
```
auto && tmp = EXPR
```

Add const if EXPR is rvalue

auto && rng = add_const_if< boost::is_rvalue_reference<decltype( (EXPR) )&&> >(tmp)

Do the usual range-based for loop over rng

libs/foreach/test/ ran successfully on gcc (4.3 - 4.6 and 4.7 (experimental)) and clang (3.1(trunk) and Apple version 2.1) both in C++03 and C++11 modes. I also tested libs/spirit/test/karma/actions.cpp on gcc-4.4 in a C++11 mode and it ran fine.

Eric Niebler — Sun, 16 Oct 2011 16:08:13 GMT

Great! I've applied the patch to both trunk and (throwing caution to the wind) to release. Thanks once again for your help.

Sun, 16 Oct 2011 17:48:05 GMT

Sorry, Eric. Could you revert the previous patch? I found a regression caused by the patch (which was not caught by the test).

I've realized that gcc-4.4 is really a mess to implement "const reference binding of rvalue ranges". We shouldn't try to implement it unless we have reliable test. Bugs in gcc-4.4 are appeared when using BOOST_FOREACH in function templates. We should extend the existing test to cover those cases.

So it seems that we have only two options for this release: A or B of comment 23.

attachment set

Sun, 16 Oct 2011 18:53:51 GMT

attachment → revert_to_cxx_03.patch

Patch that implements A of comment 23 (against trunk before applying the previous patch).

Sun, 16 Oct 2011 18:55:42 GMT

Attached a patch that implements A (reverting to the C++03 BOOST_FOREACH on gcc-4.4).

I've found that even with the simplest implementation of B

...
if (bool is_rng_defined) = false) {} else
for (auto &&rng = (EXPR); !is_rng_defined; is_rng_defined = true)
...

we can make a regression due to the nasty auto bug in gcc-4.4. So we have only one choice: A.

Eric Niebler — Sun, 16 Oct 2011 19:11:35 GMT

OK, done. Thanks again.

Mon, 17 Oct 2011 15:17:33 GMT

On gcc-4.3 (C++11), I found a regression that was not caught by the test. Due to auto type deduction bugs (decltype) in template functions, the following code does not compile on gcc-4.3 (C++11) with Boost trunk

#include <iostream>
#include <vector>
#include <boost/foreach.hpp>
template<typename T>
typename boost::mpl::if_<boost::is_rvalue_reference<T&&>, T const, T>::type
add_const_if_rvalue(T&& t)
{
    return t;
}
template <typename T>
void f()
{
    BOOST_FOREACH(int const& i, add_const_if_rvalue(std::vector<int>(3, 0))) {
        std::cout << i << std::endl;
    }
}
int main (int argc, char* argv[])
{
    f<void>();
    return 0;
}

This code compiles fine on gcc-4.3 (C++11) with Boost.1.47. It also compiles fine on gcc-4.5, 4.6, 4.7 (C++11) with Boost trunk.

Do we need to revert to the C++03 BOOST_FOREACH on gcc-4.3 too? Patch is pretty simple.

boost/foreach.hpp

 // With these C++0x features, temporary collections can be bound to
 // rvalue references and their lifetime is extended. No copy/move is needed.
 #if !defined(BOOST_NO_DECLTYPE) && !defined(BOOST_NO_RVALUE_REFERENCES)                          \
  && !(BOOST_WORKAROUND(__GNUC__, == 4) && (__GNUC_MINOR__ == 4) && !defined(BOOST_INTEL) &&      \
+ && !(BOOST_WORKAROUND(__GNUC__, == 4) && (__GNUC_MINOR__ <= 4) && !defined(BOOST_INTEL) &&      \
                                                                    !defined(BOOST_CLANG))
 # define BOOST_FOREACH_USE_RVALUE_REFERENCE_BINDING
 // Some compilers let us detect even const-qualified rvalues at compile-time

Eric Niebler — Tue, 18 Oct 2011 03:55:20 GMT

Thanks for your thoroughness on this issue. It's much appreciated. I've made your suggested change on trunk and release.

Thu, 20 Oct 2011 16:34:58 GMT

*Regressions for compilers supporting C++11 lambda*

Because the current C++11 BOOST_FOREACH uses decltype rather than auto, it cannot be used with C++11 lambda. This is possibly a serious regression. The following code compiles fine on VC++10 and gcc 4.5-4.7 with Boost 1.47, but fails to compile with Boost trunk:

#include <iostream>
#include <vector>
#include <boost/foreach.hpp>
#define BOOST_RESULT_OF_USE_DECLTYPE
#include <boost/range/adaptor/transformed.hpp>
int main (int argc, char* argv[])
{
    std::vector<int> v(3, 0);
    BOOST_FOREACH(int x, v | boost::adaptors::transformed([](int i){ return i + 1; }))
    {
        std::cout << x << std::endl;
    }
    return 0;
}

I know the release branch is already closed, but how about deferring C++11 support to Boost 1.49?

Sorry for repeatedly troubling you. I should have noticed this regression earlier and I shouldn't have used decltype to workaround gcc's auto bugs.

Thu, 20 Oct 2011 23:37:41 GMT

To support the C++11 BOOST_FOREACH, we need to address the following:

Extending test cases

Especially, use BOOST_FOREACH in function templates and use BOOST_FOREACH with C++11 lambda.

Workaround gcc's auto declaration bugs in function templates

Except gcc 4.7, gcc's auto declarations in function templates do not work correctly. Even with gcc 4.6.1,
    auto&& rng = add_const_if_rvalue(std::vector<int>(3, 0));
fails to compile (in function templates).

It is possible that compilers have other unkown bugs on auto declarations, and so it might be reasonable to have a way to avoid regressions:

Adding configuration macro BOOST_FOREACH_USE_AUTO_DECLARATIONS

The C++11 BOOST_FOREACH gets enabled on compilers with rvalue references and auto support, only if BOOST_FOREACH_USE_AUTO_DECLARATIONS is defined by users.

status changed; resolution deleted

Eric Niebler — Fri, 21 Oct 2011 03:52:19 GMT

status closed → reopened
resolution fixed

This change has made me nervous, so I have completely reverted all changes for the 1.48 release. We'll give it another go in 1.49.

attachment set

Tue, 08 Nov 2011 20:05:16 GMT

attachment → foreach_use_cxx11.patch

A patch (against trunk) that implements C++11 BOOST_FOREACH and workarounds for auto declaration bugs in gcc 4.4-4.6.

attachment set

Tue, 08 Nov 2011 20:09:03 GMT

attachment → foreach_add_new_tests.patch

A patch (against trunk) that adds tests for gcc bugs and C++11 lambdas. This patch also adds tests for noncopyable rvalue collections.

Tue, 08 Nov 2011 20:47:07 GMT

Finally, I came up with workarounds for gcc's auto type deduction bugs. Now BOOST_FOREACH consists of three implementations: C++03 BOOST_FOREACH, C++11 BOOST_FOREACH and workarounds for gcc bugs. All the existing tests and new tests (involving function templates and C++11 lambdas) ran successfully on gcc and clang.

To workaround gcc bugs, I used a hybrid of C++11 and C++03 codes:

Bind a const reference to a range using auto const&. If the range is non-const lvalue, constness of the reference is stripped off via const_cast when accessing the reference.
begin and end iterators are stored as auto_any_t to avoid the bugs.

This workaround code is enabled in C++11 modes of gcc 4.4-4.6.

On MSVC++ 10, I changed to use the C++03 BOOST_FOREACH code. Since this compiler doesn't support iteration over noncopyable rvalue ranges, it's not worth taking a risk for regressions.

I also added a few tests:

noncopyable_rvalue_*.cpp: This test emits expected-failures if BOOST_FOREACH_USE_CXX11 or BOOST_FOREACH_USE_CXX11_GCC_WORKAROUND is not defined.
cxx11_gcc_workaround_*.cpp: This test needs support of rvalue references. On compilers without rvalue reference support, this test succeeds unconditionally.
cxx11_lambda_*.cpp: This test needs support of C++11 lambdas. On compilers without C++11 lambdas support, this test succeeds unconditionally.
char_array_fail_*.cpp: A test case that should fail to compile (compile-fail is specified in Jamfile). This test tries to iterating over a char array.
rvalue_nonconst_access_fail_*.cpp: A test case that should fail to compile (compile-fail is specified in Jamfile). This test tries to capture elements in an rvalue range by non-const references.

Patches for boost/foreach.hpp and libs/foreach/test/ are attached.

attachment set

Michel Morin — Sun, 03 Jun 2012 08:01:33 GMT

attachment → foreach_use_cxx11_v2.patch

Updated patch of foreach_use_cxx11.patch. With this patch, we can iterate over noncopyable temporary ranges on VC++ 11 as well.

Michel Morin — Sun, 03 Jun 2012 08:04:27 GMT

I confirmed that VC++ 11 (beta) fixed the bug in reference binding of noncopyable temporaries. So we can iterate over noncopyable rvalue ranges on VC++ 11. I ran the tests on VC++11 with my previous patch, but some tests failed because of VC++'s bug in two-phase name look-up.

So, again, I updated the patch. Now, all the tests (incl. tests for noncopyable/nonmovable rvalue ranges and C++11 lambdas) passed on VC++ 11, gcc 4.4-4.7, clang 2.9-3.1.

Updated patch attached in this ticket: https://svn.boost.org/trac/boost/attachment/ticket/5475/foreach_use_cxx11_v2.patch

Also, patch for the tests attached in this ticket: https://svn.boost.org/trac/boost/attachment/ticket/5475/foreach_add_new_tests.patch

Michel Morin — Thu, 16 Aug 2012 06:57:29 GMT

Before gcc 4.7.1 and gcc 4.8.0 (not yet released), auto type deduction in function templates is done differently than the one in non-template functions. This resulted in many bugs in gcc 4.4-4.6 for auto type deduction in function templates. So my patch does workarounds for those compilers.

In gcc 4.7.0, I don't know of any bugs that affect (normal) C++11 implementation of BOOST_FOREACH. But gcc 4.7.0 still contains bugs of auto type deduction in function templates (c.f. PR53484 ("Wrong auto in lambdas in function templates")), and there might be a bug that needs workarounds as in gcc 4.4-4.6.

Would it be better to do workarounds for gcc 4.7.0, too? (My patch does not apply workarounds for gcc 4.7.0.)

Eric Niebler — Thu, 16 Aug 2012 08:04:48 GMT

I'll be honest with you, Michel. At this point, given the sheer size and complexity of your patch, and the number of broken compilers that half-implement C++11 that I need to support, and the past difficulties we experienced trying to get this fix to fly, it's looking highly doubtful that I'll be taking this patch. Especially considering that C++11 users have a better option: use C++11's range-based for. I can tell you've invested a lot of effort into this, and I greatly appreciate it.

I would very much like to move BOOST_FOREACH into maintenance mode. It's a relic. I don't expect I'll be adding any new features to it.

I'll leave the bog open for now and may reconsider. But for now, I wouldn't spend any more time on this if I were you.

Michel Morin — Thu, 16 Aug 2012 08:30:37 GMT

I would very much like to move BOOST_FOREACH into maintenance mode. It's a relic. I don't expect I'll be adding any new features to it.

I think it's reasonable that BOOST_FOREACH becomes in a maintenance mode.

I'll leave the bog open for now and may reconsider. But for now, I wouldn't spend any more time on this if I were you.

OK. I'll dive into the C++11 world as you do rather than struggle with half-broken C++11 compilers. That would be more fun ;)

Thanks for your reply, Eric!

status changed; resolution set

Eric Niebler — Fri, 16 Nov 2012 00:03:15 GMT

status reopened → closed
resolution → wontfix

Closing this out. I won't be adding any more features to BOOST_FOREACH. Thanks for your efforts, Michel.