Opened 10 years ago

Closed 8 years ago

#7888 closed Feature Requests (fixed)

circular_buffer should support move semantics

Reported by: holmes@… Owned by: Antony Polukhin
Milestone: Boost 1.57.0 Component: circular_buffer
Version: Boost 1.52.0 Severity: Problem
Keywords: circular_buffer move emplace Cc:

Description

I would like to use circular buffer to store unique_ptr's or other moveable objects. I hope this feature will be added.

Change History (32)

comment:1 by anonymous, 10 years ago

Elaborate please. What it is move semantics? Why you cannot store unique_ptrs in the current version?

comment:2 by holmes@…, 10 years ago

a description of the term move semantics can be found here http://www.cprogramming.com/c++11/rvalue-references-and-move-semantics-in-c++11.html

Here is an example of what I would like to do which works with a std::deque :

std::deque<std::unique_ptr<int>> myQueue;
std::unique_ptr<int> myP(new int(4));		
myQueue.push_back(std::move(myP));		//transfer ownership to the queue
auto otherP = std::move(myQueue.front());	//transfer ownership back from the queue to a new unique_ptr
myQueue.pop_front();

This doesn't work with boost::circular_buffer because there is only one overload of push_back and it takes a const T&. I need another overload that takes T&&.

comment:3 by anonymous, 10 years ago

I would also like to see this feature implemented in circular_buffer. At this moment it is actually possible to use std::shared_ptr. But, in order to maintain the same semantics as other containers in C++11 STL, circular_buffer should implement move semantics as well. Once that happens, using std::unique_ptr will be straightforward, bringing a potential performance improvement compared to std::shared_ptr.

comment:4 by anonymous, 9 years ago

One more request for this. If you store more complex elements inside the circular buffer, you don't want to have overhead code to care for the destruction of the elements when they are pushed out of the ring.

comment:5 by Antony Polukhin, 9 years ago

Owner: changed from Jan Gaspar to Antony Polukhin
Status: newassigned

comment:6 by Antony Polukhin, 9 years ago

(In [84941]) Basic commit of C++11 move constructor and move assignment for circular_buffer (refs #7888). some of the functions marked with BOOST_NOEXCEPT

comment:7 by Antony Polukhin, 9 years ago

(In [84984]) Add basic rvalues move support for elements of circular_buffer (refs #7888). Patch uses Boost.Move to emulate rvalues in C++03

comment:8 by Antony Polukhin, 9 years ago

(In [84988]) Improved rvalues move support for elements of circular_buffer (refs #7888):

  • move_if_noexcept uses is_copy_constructible trait
  • more tests, tests are now more strict
  • linearize() now works with move-only types

comment:9 by Antony Polukhin, 9 years ago

(In [84991]) Improved rvalues move support for elements of circular_buffer (refs #7888):

  • set_capacity and rset_capacity now work with move-only types if move constructor of type marked with noexcept
  • force placement ::new usage
  • minor optimizations (move values in more cases)
  • more tests

comment:10 by Antony Polukhin, 9 years ago

(In [85003]) Improved rvalues move support for elements of circular_buffer (refs #7888):

  • all erase methods now use move construction to to move elements
  • space optimized circullar buffer now has move constructor, move assignment and functions that work with rvalues
  • more methods marked as BOOST_NOEXCEPT
  • much more tests

comment:11 by Antony Polukhin, 9 years ago

(In [85059]) Fix errors in circular_buffer tests(refs #7888).

comment:12 by Antony Polukhin, 9 years ago

(In [85082]) Fixed my own typo (refs #7888)

comment:13 by Antony Polukhin, 9 years ago

(In [85101]) Fixed MSVC related bug for rvalues move support of circular_buffer (refs #7888)

comment:14 by Antony Polukhin, 9 years ago

(In [85133]) Fix errors in circular_buffer tests(refs #7888).

comment:15 by Antony Polukhin, 9 years ago

(In [85240]) Updated documentaion of the circular_buffer to reflect the rvalue references support (refs #7888) + replaced some tabs with whitespaces and added the boost.root parameter to jamfile.v2

comment:16 by Antony Polukhin, 9 years ago

(In [85458]) Make move_if_noexcept more strict and move values only if they have noexcept move constructor *and* noexcept move assignment operator (refs #7888)

comment:17 by Antony Polukhin, 9 years ago

Resolution: fixed
Status: assignedclosed

(In [85510]) Big merge of Boost.CircularBuffer :

  • Full merge of QuickBoock documentation from Paul A. Bristow
  • Merged rvalue references support with tests and documentation (fixed #7888)

comment:18 by pbf@…, 9 years ago

This change has unfortunately introduced fresh warnings when building against boost 1.55 using VisualStudio 2010 with warning level 4. I now see:

1>C:\src\svn\OTI\ThirdParty\boost_1_55_0_OT1\include\boost/circular_buffer/base.hpp(199): warning C4172: returning address of local variable or temporary

Likewise, building using gcc 4.4.7 on RedHat produces the following warning:

/media/sf_C_DRIVE/src/svn/OTI/ThirdParty/boost_1_55_0_OT1/include/boost/circular_buffer/base.hpp:2157: error: suggest parentheses around ‘&&’ within ‘

Shall I open a new ticket for this issue? Unfortunately it's causing my builds to break, since I build with warnings-as-errors on both platforms. I'll need to either disable the warnings, or fix the problems.

in reply to:  18 comment:19 by Antony Polukhin, 9 years ago

Replying to pbf@…:

This change has unfortunately introduced fresh warnings when building against boost 1.55 using VisualStudio 2010 with warning level 4. I now see:

Great thanks for reporting that issue! I'll take care of those warnings, no need to open a separate ticket.

comment:20 by Antony Polukhin, 9 years ago

Resolution: fixed
Status: closedreopened

comment:21 by michael.broida@…, 8 years ago

We have hit the same NEW warnings in circular_buffer in Boost 1.55.0 with VS2010 and all warnings treated as errors. This issue shows "reopened", but that was six months ago and no more info. Any indication of when it will be fixed, and/or a patch we can apply ourselves?

in reply to:  21 comment:22 by Antony Polukhin, 8 years ago

Replying to michael.broida@…:

We have hit the same NEW warnings in circular_buffer in Boost 1.55.0 with VS2010 and all warnings treated as errors. This issue shows "reopened", but that was six months ago and no more info. Any indication of when it will be fixed, and/or a patch we can apply ourselves?

Warning with parentheses is fixed in develop and master branches

Other warning is not fixed. The simplest workaround would be to disable this warning at all.

Correct solution would be to move the move_if_noexcept into the Boost.Move module and take care of it there. Unfortunately I had no time to take care of that issue.

comment:23 by michael.broida@…, 8 years ago

OK. The unfixed "returning address of local variable or temporary" error is the one I'm seeing. It looks like only one line in our code leads to that warning, so I will disable that warning for that single line.

I hope you (or someone) has time to make the full fix before the next release. :) Thank you. :)

in reply to:  23 comment:24 by Antony Polukhin, 8 years ago

Replying to michael.broida@…:

only one line in our code leads to that warning

Could you please provide that line here? What function of the circular_buffer was called?

comment:25 by michael.broida@…, 8 years ago

Apparently it's just the circular_buffer ctor.
(scopenames removed to make it easier to read.)

struct TestSetupContainers
{
  TestSetupContainers()
  {
    output_object_pool_ = new GrowingMemoryPool<sizeof(OutputObject)>(11,0, "TestSetupContainers MemoryPool");
    OutputObject::SetMemoryPool(output_object_pool_);
    output_buffer_.resize(10);
  }

  GrowingMemoryPool<sizeof(OutputObject)> *output_object_pool_;
  boost::circular_buffer<boost::shared_ptr<OutputObject>> output_buffer_;
};

That last line is in our file OutputObject-UT.cpp line 20, the line triggering this long warning:
(This is with VS2010; Boost 1.55.0)
(long paths shortened to ....)

1>  OutputObject-UT.cpp
1>....include\boost\circular_buffer\base.hpp(201): error C2220: warning treated as error - no 'object' file generated
1>          ....include\boost\circular_buffer\base.hpp(2199) : see reference to function template instantiation 'const boost::shared_ptr<T> &boost::circular_buffer<boost::shared_ptr<T>>::move_if_noexcept<boost::shared_ptr<T>>(ValT &)' being compiled
1>          with
1>          [
1>              T=OutputObject,
1>              ValT=boost::shared_ptr<OutputObject>
1>          ]
1>          ....include\boost\circular_buffer\base.hpp(2191) : while compiling class template member function 'boost::cb_details::iterator<Buff,Traits> boost::circular_buffer<T>::erase(boost::cb_details::iterator<Buff,Traits>,boost::cb_details::iterator<Buff,Traits>)'
1>          with
1>          [
1>              Buff=boost::circular_buffer<boost::shared_ptr<OutputObject>>,
1>              Traits=boost::cb_details::nonconst_traits<std::allocator<OutputObjectHandle>>,
1>              T=boost::shared_ptr<OutputObject>
1>          ]
1>          ....outputobject-ut.cpp(20) : see reference to class template instantiation 'boost::circular_buffer<T>' being compiled
1>          with
1>          [
1>              T=boost::shared_ptr<OutputObject>
1>          ]
1>....include\boost\circular_buffer\base.hpp(201): warning C4172: returning address of local variable or temporary

I -was- able to avoid the warning by #pragma disabling it for the entire method in Boost's circular_buffer\base.hpp lines 189-200. That just seems like cheating, though. :) (I can't even figure out that method name!)

I hope that helps. Let me know if you need more information.

comment:26 by michael.broida@…, 8 years ago

Oops. That error message was from a version where I was experimenting with code mods. With NO mods from Boost 1.55.0, the messages point to different lines:

  • "base.hpp(201)" should be "base.hpp(199)" (two places)
  • "base.hpp(2199)" should be "base.hpp(2196)"
  • "base.hpp(2191)" should be "base.hpp(2188)"

Sorry for that glitch.

comment:27 by anonymous, 8 years ago

Ah-ha! I did some further testing.\ Though all the messages point to the line declaring the circular_buffer, the warnings only appear when the TestSetupContainer struct's ctor contains this line:

output_buffer_.resize(10);

So it would appear that the call to "resize" actually triggers the warnings.
I commented out everything in that test file except that struct declaration (and necessary #includes), just to be sure.

I also changed the declaration of "output_buffer_" to these types:

"output_buffer_" typeresult
boost::circular_buffer<int>NO warning
boost::circular_buffer<boost::shared_ptr<int>>SAME warning
boost::circular_buffer<std::shared_ptr<int>>SAME warning

If I think of anything else, I'll post more here.

comment:28 by michael.broida@…, 8 years ago

Aaargh! Now I'm very confused.
Forget that bit about the "resize" call. NOW I get the warning whether that line is present or not.
I know it went away when I commented that line out earlier. But now it's back. And the last two alternate types in the table in my previous post worked the same way. But now they don't.
I think Visual Studio 2010 hates me. :(

in reply to:  26 comment:29 by Antony Polukhin, 8 years ago

Replying to michael.broida@…:

With NO mods from Boost 1.55.0, the messages point to different lines:

  • "base.hpp(201)" should be "base.hpp(199)" (two places)
  • "base.hpp(2199)" should be "base.hpp(2196)"
  • "base.hpp(2191)" should be "base.hpp(2188)"

Those lines look pretty innocent. move_if_noexcept is used there for non temporaries and must not cause any troubles.

I think Visual Studio 2010 hates me. :(

Sometimes I think that there is a hidden "Do respect the programmer!" switch somewhere in Windows registry (in Linux it's on by default). Please tell me if you'll find it! :)

comment:30 by Matthäus Brandl <brandl.matthaeus@…>, 8 years ago

Using Boost 1.56.0 warning C4172 is still reported by VS2010.

comment:31 by Antony Polukhin, 8 years ago

Milestone: To Be DeterminedBoost 1.57.0

I hope that the last warnings were fixed by this commit, which is already merged to release branch of 1_57_0.

comment:32 by Antony Polukhin, 8 years ago

Resolution: fixed
Status: reopenedclosed
Note: See TracTickets for help on using tickets.