templated overload of boost::lockfree::spsc_queue::pop is a potential footgun

[tjakubowski@…]

boost::lockfree::spsc_queue::pop has many overloads. I've seen code that to tries to select this one:

template<typename U> 
  boost::enable_if< typename is_convertible< T, U >::type, bool >::type 
  pop(U & ret);

but, by mistake, selects this one:

template<typename OutputIterator> 
  boost::disable_if< typename is_convertible< T, OutputIterator >::type, size_type >::type 
  pop(OutputIterator it);

How? The problematic code looks like this:

boost::spsc_queue<Thing> q; 

void consumer()
{
  for (;;) {
    while (!q.empty()) {
      OtherThing foo = ...
      Thing bar;
      AnotherThing baz = ...;  
      q.pop(&bar);
      // do something with foo, bar, and baz
    }
  }
}

Note the & operator, which will select the templated "pop to an output iterator" overload because pointers to non-const are output iterators, rather than lead to a compilation error (as one might expect). This is a nasty bug because when there isn't much pressure on the queue, and at most one element is present, the consumer will appear to work correctly. But if there is more than one element in the queue, popping all of them off will cause undefined behavior (probably overwriting the memory of foo or baz in the above example) that, speaking from experience, is pretty hard to nail down.

My request is to deprecate this templated "pop to an iterator" overload, and make a new member function called pop_all with the same signature to replace it. This would help to prevent this mistake, and also make the API somewhat more consistent with the consume_one/consume_all functions.

[tjakubowski@…]

Replying to tjakubowski@…:

boost::spsc_queue<Thing> q; 

Oops, I meant to say boost::lockfree::spsc_queue here, of course.