infix_iterator code

Question

I've previously posted this on Stack Overflow, and am considering submitting it to Boost for wider distribution, but thought perhaps it would be best to put it up here for peer review first, and see whether there are clear improvements that can be made first.

// infix_iterator.h
// 
#if !defined(INFIX_ITERATOR_H_)
#define  INFIX_ITERATOR_H_
#include <ostream>
#include <iterator>

template <class T,
          class charT=char,
          class traits=std::char_traits<charT> >

class infix_ostream_iterator :
    public std::iterator<std::output_iterator_tag,void,void,void,void>
{
    std::basic_ostream<charT,traits> *os;
    charT const* delimiter;
    bool first_elem;
public:
    typedef charT char_type;
    typedef traits traits_type;
    typedef std::basic_ostream<charT,traits> ostream_type;

    infix_ostream_iterator(ostream_type& s)
        : os(&s),delimiter(0), first_elem(true)
    {}
    infix_ostream_iterator(ostream_type& s, charT const *d)
        : os(&s),delimiter(d), first_elem(true)
    {}
    infix_ostream_iterator<T,charT,traits>& operator=(T const &item)
    {
        // Here's the only real change from ostream_iterator:
        // We don't print the delimiter the first time. After that, 
        // each invocation prints the delimiter *before* the item, not
        // after. As a result, we only get delimiters *between* items,
        // not after every one.
        if (!first_elem && delimiter != 0)
            *os << delimiter;
        *os << item;
        first_elem = false;
        return *this;
    }

    infix_ostream_iterator<T,charT,traits> &operator*() {
        return *this;
    }
    infix_ostream_iterator<T,charT,traits> &operator++() {
        return *this;
    }
    infix_ostream_iterator<T,charT,traits> &operator++(int) {
        return *this;
    }

};

#endif 

This is (at least intended to be) pretty much a drop-in replacement for std::ostream_iterator, the sole difference being that (at least in normal use) it only prints out delimiters between items instead of after every item. Code using it looks something like:

#include "infix_iterator.h"

std::vector<int> numbers = {1, 2, 3, 4};

std::copy(begin(numbers), end(numbers), 
          infix_ostream_iterator<int>(std::cout, ", "));

The motivation for this is pretty simple -- with a std::ostream_iterator, your list would come out like 1, 2, 3, 4,, but with the infix_iterator, it comes out as 1, 2, 3, 4.

As a side-note, although I've used a couple of C++11 features in this demo code, I believe the iterator itself should be fine with C++03 -- though if somebody sees anything that would be a problem for a compiler without C++11 support, I'd like to hear about that too.

Edit: In case anybody cares, here's the new version incorporating input from both @Konrad and @Loki. Thanks to both of you.

// infix_iterator.h
#if !defined(INFIX_ITERATOR_H_)
#define  INFIX_ITERATOR_H_
#include <ostream>
#include <iterator>
#include <string>

template <class T, class charT=char, class traits=std::char_traits<charT> >
class infix_ostream_iterator :
    public std::iterator<std::output_iterator_tag, void, void, void, void>
{
    std::basic_ostream<charT,traits> *os;
    std::basic_string<charT> delimiter;
    std::basic_string<charT> real_delim;

public:

    typedef charT char_type;
    typedef traits traits_type;
    typedef std::basic_ostream<charT, traits> ostream_type;

    infix_ostream_iterator(ostream_type &s)
        : os(&s)
    {}

    infix_ostream_iterator(ostream_type &s, charT const *d)
        : os(&s), 
          real_delim(d)
    {}

    infix_ostream_iterator<T, charT, traits> &operator=(T const &item)
    {
        *os << delimiter << item;
        delimiter = real_delim;
        return *this;
    }

    infix_ostream_iterator<T, charT, traits> &operator*() {
        return *this;
    }

    infix_ostream_iterator<T, charT, traits> &operator++() {
        return *this;
    }

    infix_ostream_iterator<T, charT, traits> &operator++(int) {
        return *this;
    }
};

#endif 

Answer 1

Nothing major (just some personal opinion ones):

Consistent Spacing

    infix_ostream_iterator(ostream_type& s)
        : os(&s),delimiter(0), first_elem(true)
    {}  //    ^^^ No Space   ^^^Trailing space

Consistent type Naming

// Here we have & on the left
infix_ostream_iterator<T,charT,traits>& operator=(T const &item)

// Here we have it on the right
infix_ostream_iterator<T,charT,traits> &operator*() {

Spelling of delimter

I mistypes it a lot when writing this => delimiter

Easier to read initializer list

Just like I prefer one statement per line, I prefer one variable initialized per line in the initializer list (easier to read).

infix_ostream_iterator(ostream_type& s, charT const *d)
    : os(&s)
    , delimiter(d)
    , first_elem(true)
{}

Remove the if from the main body.

It probably makes no difference to performance, but I would remove the if so the code looks like this:

*os << delimter << item;
delimter = actualDelimter;
return *this;

On construction actualDelimter points at the string provided by the user (or empty string) and delimter is set to point at an empty string.

Answer 2

The only thing at all that I can criticise in the code is the inconsistent placement of whitespace between infix operators and in pointer / reference declarations.

class charT=char,
public std::iterator<std::output_iterator_tag,void,void,void,void>

… etc., missing spaces.

: os(&s),delimiter(0), first_elem(true)

… etc., inconsistent use of spaces.

infix_ostream_iterator<T,charT,traits>& operator=(T const &item)
infix_ostream_iterator<T,charT,traits> &operator*() {

… etc., Inconsistent placement of &.

I’d also unify the use of blank lines between function definitions, and remove the blank line between the template <…> block and the class header, as well as at the end of the class definition.

Nitpicking, sure, but that’s literally the only thing to criticise.

Answer 3

I would also declare first constructor as explicit:

explicit infix_ostream_iterator(ostream_type &s)
        : os(&s)
    {}

Though I can imagine use of this kind of iterator is rather limited, this way you ensure no unwanted type conversion will take place.

Answer 4

Keep the const char pointers

I agree with Lokis proposed changes, except that I would leave delimiter and real_delim as charT const * and change the constructor to:

infix_ostream_iterator(ostream_type &s, charT const *d = 0)
 : os(&s)
 , delimiter("")
 , real_delim(d ? d : "")
{
}

That way we reduce the cost of copying the strings, instead we just copy a pointer.

Deduced types

Mimicking the changes made to std::less in C++14, you could make operator= a template, that way we don't need to specify this type for the class. This would though break the compatibility with the std::ostream_iterator interface.

template <class T>
infix_ostream_iterator<charT, traits> &operator=(T const &item)
{
    *os << delimiter << item;
    delimiter = real_delim;
    return *this;
}

Answer 5

First of all, good idea with this class! I agree with the changes already proposed by other users (Loki, Konrad, user15108, dalle).

Additionally, I noticed that infix_ostream_iterator is not default constructible. This is fine for the use case and follows the design of std::ostream_iterator. Therefore you might as well store the std::basic_ostream as a reference member variable. That is,

class infix_ostream_iterator : ... {
    ...
    std::basic_ostream<charT,traits>& os;
    ...
}

Now you can remove the pointer indirection which will simplify the remaining code. E.g., the constructor becomes

infix_ostream_iterator(ostream_type &s, charT const *d)
    : os(s)
    , real_delim(d)
{}

and you can directly write to the stream

os << delimiter << item;

This is an implementation detail (your interface is the same). I reckon that the compiler will generate the same set of instructions anyhow.

Looking forward to seeing this class in something like Boost! Good luck with the submission.

Answer 6

I know I'm years late on this but I want to throw out an idea: why do the delimiter and non-delimiter cases have to be the same type? Your initial version used charT const* and your new version uses std::basic_string<charT>... but it seems wasteful to have two strings that we're copying just to handle not having a delimiter.

I'd suggest having an overloaded make_* function that either creates your iterator or creates the standard one:

// no delimiter
template <class T, class CharT, class Traits>
std::ostream_iterator<T, CharT, Traits>
make_delim_iterator(std::basic_ostream<CharT,Traits>& );

// yes, delimiter
template <class T, class CharT, class Traits>
infix_ostream_iterator<T, CharT, Traits>
make_delim_iterator(std::basic_ostream<CharT,Traits>&, CharT const*);

This will allow you to have two member CharT const*'s instead of two basic_string<CharT>'s, and your type just always uses a delimiter.