Modify

Opened 4 years ago

Closed 4 years ago

#8674 closed Bugs (fixed)

Futures as local named objects can't be returned with implicit move.

Reported by: mjklaim@… Owned by: viboes
Milestone: Boost 1.54.0 Component: thread
Version: Boost 1.53.0 Severity: Problem
Keywords: Cc:

Description

I could test only with VS 2012 Update 2 so this might be related to a compiler bug, but I can't test the boost code with other compilers/platforms right now.

I have this simple test:

#include <iostream>

#define USE_STD 0
#define USE_BOOST 1

#define USED_THREAD_API USE_BOOST

#if USED_THREAD_API == USE_BOOST

#	define BOOST_THREAD_VERSION 4
#	include <boost/thread/future.hpp>

	using boost::future;
	using boost::async;

#endif
#if USED_THREAD_API == USE_STD
#	include <future>
	using std::future;
	using std::async;
#endif



future<void> do_something()
{
	auto result = async( []{ std::cout<< "A\n"; } );
	std::cout << "B\n";
	return result; // error here
}

int main()
{
	do_something().wait();
	std::cout << "Hello, World!" << std::endl;

}

Both default Debug and Release modes fail to compile on VS2012 U2:

1>------ Build started: Project: Test_MoveReturnFuture, Configuration: Debug Win32 ------
1>  main.cpp
1>e:\projects\tests\test_movereturnfuture\test_movereturnfuture\main.cpp(29): error C2248: 'boost::future<R>::future' : cannot access private member declared in class 'boost::future<R>'
1>          with
1>          [
1>              R=void
1>          ]
1>          e:\projects\sdk\boost\boost\include\boost-1_53\boost\thread\future.hpp(1406) : see declaration of 'boost::future<R>::future'
1>          with
1>          [
1>              R=void
1>          ]
========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========

Changing this line:

#define USED_THREAD_API USE_BOOST

To this:

#define USED_THREAD_API USE_STD

Makes it compile and execute fine.

I see that there are move constructors in the definition of future so I don't know at all what's the difference between the two implementations.

Another way to avoid the problem is to explicitly move the future:

future<void> do_something()
{
	auto result = async( []{ std::cout<< "A\n"; } );
	std::cout << "B\n";
	return std::move(result);
}

Which is what I have been doing in my code because I was initially thinking it was a compiler bug, but this Q/A made me reconsider: http://stackoverflow.com/questions/4986673/c11-rvalues-and-move-semantics-confusion

I cannot test Boost 1.54 at the moment.

Attachments (1)

std_future.hpp (51.9 KB) - added by mjklaim@… 4 years ago.
VS2012 U2 <future> implementation

Download all attachments as: .zip

Change History (16)

comment:1 Changed 4 years ago by viboes

Owner: changed from Anthony Williams to viboes
Status: newassigned

IMO, the code is correct. I have tested it with gcc-4.7.2/4.8.0, clang-3.2 and it works as expected with both defines USED_THREAD_API USE_BOOST or USED_THREAD_API USE_STD.

Could you show the implementation of std::future constructors of the library you are using?

Changed 4 years ago by mjklaim@…

Attachment: std_future.hpp added

VS2012 U2 <future> implementation

comment:2 Changed 4 years ago by mjklaim@…

I copy/pasted the <future> file code into the attached one. It's the one provided with VS2012 U2. I don't use the community betas.

Note that the problem might be a VS compiler bug that might be fixed in the community beta. If someone can test the same code with the community beta it would help.

comment:3 in reply to:  1 Changed 4 years ago by viboes

Replying to viboes:

IMO, the code is correct. I have tested it with gcc-4.7.2/4.8.0, clang-3.2 and it works as expected with both defines USED_THREAD_API USE_BOOST or USED_THREAD_API USE_STD.

Could you show the implementation of std::future constructors of the library you are using?

I have tried with VS 2010, but there is no <future> header. I confirm the bug with VS 2010 and #define USED_THREAD_API USE_BOOST. Unfortunately I've a windows XP and I can not install VS 2012 :(

Last edited 4 years ago by viboes (previous) (diff)

comment:4 Changed 4 years ago by mjklaim@…

Note that I'm using VS2012 Update 2 but there is Update 3 Release Candidate. The log don't seem to have anything related to this bug.

Also, I'm using 64 and 32 bits and have the error whatever the case.

comment:5 Changed 4 years ago by mjklaim@…

I asked on a french C++ forum if someone could test with VS2012 November CTP1 (which add several c++11 features but can't be used safely in production).

Loic Joly reported the same error (which copying here);

1>------ Début de la génération*: Projet*: TestFutureCTP (Microsoft Visual C++ Compiler Nov 2012 CTP), Configuration*: Debug Win32 ------
1>  'Microsoft Visual C++ Compiler Nov 2012 CTP' is for testing purposes only.
1>  Source.cpp
1> ****\visual studio 2012\projects\testfuturectp\source.cpp(29): error C2248: 'boost::future<void>::future' : cannot access private member declared in class 'boost::future<void>'
1>          d:\boost\boost_1_53_0\boost\thread\future.hpp(1406) : see declaration of 'boost::future<void>::future'

comment:6 in reply to:  2 Changed 4 years ago by viboes

Replying to mjklaim@…:

I copy/pasted the <future> file code into the attached one. It's the one provided with VS2012 U2. I don't use the community betas.

Note that the problem might be a VS compiler bug that might be fixed in the community beta. If someone can test the same code with the community beta it would help.

I wonder if the following constructor is the one that is been used by the compiler.

1126	        future(const _Mybase& _State)
1127	                : _Mybase(_State, true)
1128	                {       // construct from associated asynchronous state object
1129	                }

This will clearly be a compiler bug (and indirectly a library bug).

Could you try adding this public constructor to Boost.Thread to confirm it

        BOOST_THREAD_FUTURE(base_type const& b):
          base_type(b.future_)
        {
        }

comment:7 Changed 4 years ago by mjklaim@…

Could you try adding this public constructor to Boost.Thread to confirm it

If I add this code it don't compile and give the same exact error. But the error points to this line:

BOOST_THREAD_MOVABLE_ONLY(BOOST_THREAD_FUTURE)

Obviously, removing it (and keeping the modification) makes it compile.

I don't know if it works though, I would have to recompile boost.thread to check on runtime. Isn't this modification duplicating the value?

comment:8 Changed 4 years ago by mjklaim@…

I checked by debugging step-by-step the std::future behaviour and it does use the move constructor, not the other one. So no, it's not using the base-copy constructor.

Maybe boost.thread futres's move-only macro is hiding the move constructor in some ways?

comment:9 Changed 4 years ago by mjklaim@…

Ok I found something. In delete.hpp:

/**
 * BOOST_THREAD_DELETE_COPY_CTOR deletes the copy constructor when the compiler supports it or
 * makes it private.
 *
 * BOOST_THREAD_DELETE_COPY_ASSIGN deletes the copy assignment when the compiler supports it or
 * makes it private.
 */
#ifndef BOOST_NO_CXX11_DELETED_FUNCTIONS
#define BOOST_THREAD_DELETE_COPY_CTOR(CLASS) \
      CLASS(CLASS const&) = delete; \

#define BOOST_THREAD_DELETE_COPY_ASSIGN(CLASS) \
      CLASS& operator=(CLASS const&) = delete;

#else // BOOST_NO_CXX11_DELETED_FUNCTIONS
#define BOOST_THREAD_DELETE_COPY_CTOR(CLASS) \
    private: \
      CLASS(CLASS&); \             // <--- HERE
    public:

#define BOOST_THREAD_DELETE_COPY_ASSIGN(CLASS) \
    private: \
      CLASS& operator=(CLASS&); \  // <--- HERE
    public:
#endif // BOOST_NO_CXX11_DELETED_FUNCTIONS

Notice that the non-c++11 implementation define private copy constructor and assignation using non-const references.

#define BOOST_THREAD_DELETE_COPY_CTOR(CLASS) \
    private: \
      CLASS(CLASS const &); \             // <--- HERE
    public:

#define BOOST_THREAD_DELETE_COPY_ASSIGN(CLASS) \
    private: \
      CLASS& operator=(CLASS const &); \  // <--- HERE
    public:

This compiles for me. (I just can't run it right now because it requires recompiling boost.thread)

comment:10 Changed 4 years ago by mjklaim@…

Just to clarify: that it works without modification of the future class.

So basically, these private constructor/assignation were selected as the right match instead of the moving constructor/assignation, just because they had non-const reference. I'm a bit lost on if it's a compiler bug or not.

comment:11 Changed 4 years ago by viboes

Thanks for the analysis. I have checked the const reference patch with VS 2010 and it works now.

I will run the whole regression test before committing these changes.

Thanks again, Vicente

comment:12 Changed 4 years ago by mjklaim@…

I'm happy to help. :)

comment:13 Changed 4 years ago by viboes

Milestone: To Be DeterminedBoost 1.54.0

comment:14 Changed 4 years ago by viboes

Committed in [84718]

comment:15 Changed 4 years ago by viboes

Resolution: fixed
Status: assignedclosed

Committed revision [84750].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain viboes.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.