Modify

Ticket #7888 (closed Feature Requests: fixed)

Opened 3 years ago

Last modified 10 months ago

circular_buffer should support move semantics

Reported by: holmes@… Owned by: apolukhin
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.

Attachments

Change History

comment:1 Changed 3 years ago by anonymous

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

comment:2 Changed 3 years ago by holmes@…

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 Changed 2 years ago by anonymous

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 Changed 2 years ago by anonymous

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 Changed 2 years ago by apolukhin

  • Owner changed from jano_gaspar to apolukhin
  • Status changed from new to assigned

comment:6 Changed 2 years ago by apolukhin

(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 Changed 2 years ago by apolukhin

(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 Changed 2 years ago by apolukhin

(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 Changed 2 years ago by apolukhin

(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 Changed 2 years ago by apolukhin

(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 Changed 2 years ago by apolukhin

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

comment:12 Changed 2 years ago by apolukhin

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

comment:13 Changed 2 years ago by apolukhin

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

comment:14 Changed 2 years ago by apolukhin

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

comment:15 Changed 2 years ago by apolukhin

(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 Changed 2 years ago by apolukhin

(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 Changed 2 years ago by apolukhin

  • Status changed from assigned to closed
  • Resolution set to fixed

(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 follow-up: ↓ 19 Changed 22 months ago by pbf@…

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.

comment:19 in reply to: ↑ 18 Changed 22 months ago by apolukhin

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 Changed 22 months ago by apolukhin

  • Status changed from closed to reopened
  • Resolution fixed deleted

comment:21 follow-up: ↓ 22 Changed 15 months ago by 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?

comment:22 in reply to: ↑ 21 Changed 15 months ago by apolukhin

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 follow-up: ↓ 24 Changed 15 months ago by michael.broida@…

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. :)

comment:24 in reply to: ↑ 23 Changed 15 months ago by apolukhin

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 Changed 15 months ago by michael.broida@…

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 follow-up: ↓ 29 Changed 15 months ago by michael.broida@…

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 Changed 15 months ago by anonymous

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 Changed 15 months ago by michael.broida@…

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. :(

comment:29 in reply to: ↑ 26 Changed 15 months ago by apolukhin

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 Changed 11 months ago by Matthäus Brandl <brandl.matthaeus@…>

Using Boost 1.56.0 warning C4172 is still reported by VS2010.

comment:31 Changed 11 months ago by apolukhin

  • Milestone changed from To Be Determined to Boost 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 Changed 10 months ago by apolukhin

  • Status changed from reopened to closed
  • Resolution set to fixed
View

Add a comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
The resolution will be deleted. Next status will be 'reopened'
Author


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

 
Note: See TracTickets for help on using tickets.