Skip to main content
3 of 3
added 87 characters in body
Barry
  • 18.6k
  • 1
  • 41
  • 92

To SFINAE or not to SFINAE

Your class template definition starts with:

template
  <
    typename T,
    typename = std::enable_if_t<std::is_nothrow_move_constructible<T>::value>,
    typename = std::enable_if_t<std::is_nothrow_destructible<T>::value>
  >
  class destruction_service final

but SFINAE is basically intended to remove candidates from an overload set. That's not really an issue here, so this seems unnecessarily verbose. What you want in this case is just the simple static asserts:

template <typename T>
class destruction_service final {
    static_assert(std::is_nothrow_move_constructible<T>::value, "!");
    static_assert(std::is_nothrow_destructible<T>::value, "!");
    ...

Locking

Your lock_queue_ is weird and unnecessary. Just construct a lock object, it'll be cleaner. Also if you find yourself writing unlock(), you're probably doing something wrong. In the case of your destructor, I would rewrite it as:

~destruction_service() noexcept
{
    syncio::print_log(__PRETTY_FUNCTION__);
    {
        std::lock_guard<std::mutex> lk(this->mutex_);
        this->done_ = true;
    }
    this->condvar_.notify_all();
    if (this->worker_.joinable())
        this->worker_.join();
    assert(this->queue_.empty());
}

The bracing makes the locking intent clear and takes advantage of RAII. A similar construction can be used for schedule_destruction:

void schedule_destruction(object_type&& object)
{
    {
        std::lock_guard<std::mutex> lk(this->mutex_);
        this->queue_.push_back(std::move(object));
    }
    this->condvar_.notify_all();
}

Prefer lock_guard to unique_lock unless you really need a unique_lock.

Lock-Free Queue

If this is an optimization, I'd consider using a lock-free queue. You may find that most of your time comes from lock acquisition/release.

do_work_()

Your loop is:

for (auto stop = false; true; stop = this->is_done_()) { ... }

That's... weird construction. Why not just:

while (!this->is_done_()) { ... }

Next, std::condition_variable::wait has an overload that takes a predicate. This clears intent a bit:

while (!this->is_done())
{
    {
        std::unique_lock<std::mutex> lk(this->mutex_); // now we need unique
        this->condvar_.wait(lk, [this]{
            // waiting for one of these things to be true
            return !this->queue_.empty() || this->done_; 
        });
        this->queue_.swap(things);
    }
    // and now we're unlocked
    things.clear();
}

// we're done, so let's clean up. no lock necessary
this->queue_.clear(); 

printer helper

Behold, the wonder of pack expansion. No template specialization with printer_helper_ necessary:

template <typename... Items>
void print(std::ostream& os, Items const&... items)
{
    std::lock_guard<std::mutex> guard{...};

    using expander = int[];
    (void)expander{0,
        (void(os << items)
        , 0)...
    };
    os << std::endl;
}

Usage of this

You don't really need this in just about all of the places you use it.

Barry
  • 18.6k
  • 1
  • 41
  • 92