Support moveable objects and allow emplacing #31
Conversation
…onstruction and emplacement
…onstruction and emplacement
…_RVALUE_REFS, add BOOST_NO_CXX11_VARIADIC_TEMPLATES guard around emplace
|
in general it looks good to me. i'd merge it, but it would be great if you could add some tests for it. |
…-only types
Also:
* Fix an issue spsc_queue::reset() would not compile for move-only types
* Fix an issue where having BOOST_NO_CXX11_RVALUE_REFERENCES defined would break compiles
|
Tests have been added to spsc_queue_test.cpp to verify the move-only and copy-only structures behave as expected. Also fixed a few issues I came across in the process. |
|
Can this please be go forward. I really need this and want to get rid of my own version (#27). |
| @@ -405,3 +406,161 @@ BOOST_AUTO_TEST_CASE( spsc_queue_reset_test ) | |||
|
|
|||
| BOOST_REQUIRE(f.empty()); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Shouldn't there also be a test for a move_only_type like unique_ptr?
| #ifndef BOOST_NO_CX11_RVALUE_REFERENCES | ||
| struct refcount_handle | ||
| { | ||
| int* p_refcount; |
There was a problem hiding this comment.
Not sure about the hungarian notation here. Consider renaming the variable.
|
@tnovotny thanks for pinging me ... i'll look into it again over the next week |
|
@timblechmann it would also be nice if you could also look at my pull request, because if I remember correctly, this will not suffice for move only types, as more of the existing API has to be disabled via I checked. I get a compile errors on |
|
@timblechmann so, here's pinging you again. |
|
@timblechmann i find it really frustrating that you are showing no effort in maintaining this library. would you please give feedback on the outstanding pull requests. |
|
@timblechmann and another two weeks. |
| @@ -112,6 +114,43 @@ class ringbuffer_base | |||
|
|
|||
| return true; | |||
| } | |||
| #ifndef BOOST_NO_CXX11_RVALUE_REFERENCES | |||
|
|
|||
| bool push(T&& t, T * buffer, size_t max_size) | |||
There was a problem hiding this comment.
You can make this work on C++98 too with Boost.Move support by using BOOST_RV_REF(T) instead of T&& combined with boost::move instead of std::move.
There was a problem hiding this comment.
i don't have a strong opinion on this: c++98 compilers are dying out and not so much new code is written for them. we just need to make sure, not to break any existing code by introducing unguarded c++1X constructs
| move_and_delete( T * first, T * last, OutputIterator out ) | ||
| { | ||
| if (boost::has_trivial_destructor<T>::value) { | ||
| return std::move(first, last, out); // will use memcpy if possible |
There was a problem hiding this comment.
Instead of duplicating this function, and using SFINAE to select the desired one, I would just have one implementation. This is safe because while, during overload resolution, the compiler will prefer move assignment, it will still fall back to copy assignment if no move assignment operator is available.
Additionally I'd use boost::move from <boost/move/algorithm.hpp> and boost::move from <boost/move/utility_core.hpp>. That will work without having to use the preprocessor (it's valid regardless of whether BOOST_NO_CXX11_RVALUE_REFERENCES is defined) and additionally supports types that implement move emulation too.
| @@ -450,6 +511,23 @@ class compile_time_sized_ringbuffer: | |||
| return ringbuffer_base<T>::push(t, data(), max_size); | |||
| } | |||
|
|
|||
| #ifndef BOOST_NO_CXX11_RVALUE_REFERENCES | |||
| bool push(T&& t) | |||
There was a problem hiding this comment.
Same as above: BOOST_RV_REF (also on C++98).
This does not apply to the variadic templates below though.
| #endif | ||
|
|
||
| #endif | ||
|
|
||
| /** Pops one object from ringbuffer. | ||
| * | ||
| * \pre only one thread is allowed to pop data to the spsc_queue |
There was a problem hiding this comment.
Just dreaming here, but:
It may be worth considering converting this argument-less pop() from returning bool to boost::optional<T>, which is convertible to bool. Because the current version, just returning bool is kind of worthless for everything except "some event has happened", similar to boost::optional<void> (if we had regular void). That would allow user code like this:
while (auto item = queue.pop())
{
// process item
}There was a problem hiding this comment.
true. one could only introduce a new API queue.pop_optional(). i'd merge a PR if you implement it :)
| * \post object will be pushed to the spsc_queue, unless it is full. | ||
| * \return true, if the push operation is successful. | ||
| * | ||
| * \note Thread-safe and wait-free |
There was a problem hiding this comment.
maybe comment about the API which is not available if BOOST_NO_CXX11_RVALUE_REFERENCES is defined
|
Its 2021 now, what is stopping this PR from being merged? There is no emplace for this library, which is an important tool to create exception safe functions. This library is essentially dead? |
not dead, but unfortunately the author cannot spend much time on it and has no use cases himself for move semantics. furthermore there were two "competing" PRs, which eventually both went stale. happy to merge any merge-ready PR, though |
|
trying to get my PR into boost::lockfree was a very frustrating experience. my PR (#27) was simply closed to go on with this one even though it did not support my use case of move only types such as unique_ptr. my changed PR (#41) received no feedback. so to me, at the time, it felt like this library was stale and eventually I just gave up. |
Is #41 merge-ready? It has no conflicts. Or, if this present PR is preferred, would resolving its merge conflicts make it merge-ready? I say this just as a random person interested in this feature. I'd be willing to clean up this current PR if that's the desired route. |
|
that was about my experience. poor to no feedback on this topic |
|
Just to weigh in, I'd also love to see this functionality merged |
|
Tim, what do you think now that C++98/03 is officially deprecated in Boost? https://pdimov.github.io/articles/phasing_out_cxx03.html My personal opinion is to stop actively supporting 98/03, so if there are users who need it then it's up to them to actively provide that support in the form of PRs, etc. And rather than surveying the community to inform a decision (which I did for another library), you'll find out pretty quickly if you just break it and see what happens. :) |
|
Are there any prospects of this being merged? |
|
for 1.86 i'll drop c++03/c++11 support and #90 will be merged |
This PR extends off of the months old #24 and it's recommended edits. Some additional work was done to make sure it also works with objects that are copy xor move constructible.