64
\$\begingroup\$

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 
\$\endgroup\$
  • \$\begingroup\$ Why do you take a charT const *d instead of a const std::basic_string<charT>&? \$\endgroup\$ – David Jul 3 '12 at 12:58
  • \$\begingroup\$ @Dave: To mimic the interface of std::ostream_iterator? \$\endgroup\$ – dalle Jan 22 '14 at 7:33
  • \$\begingroup\$ Have you submitted to Boost upstream? I can't seem to find neither infix_iterator nor infix_ostream_iterator in Boost. \$\endgroup\$ – sukhmel Nov 19 '14 at 10:56
  • \$\begingroup\$ @sukhmel: No--I've gotten somewhat caught up in a few other things, and that got dropped in a crack, so to speak. \$\endgroup\$ – Jerry Coffin Nov 19 '14 at 17:24
  • \$\begingroup\$ @JerryCoffin: that's a most useful tool; I really hate those trailing delims. Mind if I link here if use a q'n'd downsized version in answers on SO? :) \$\endgroup\$ – decltype_auto Nov 6 '15 at 18:50
24
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I've been thinking about this a bit. Instead of storing the user's pointer directly, and initializing delimiter to point to an empty string, do you see a problem that I don't with using std::basic_string<charT> for both delimiter and real_delim? Pros: no init to empty string needed, remain valid even if the user's pointer doesn't. Cons: Possibly increased overhead? \$\endgroup\$ – Jerry Coffin Jul 1 '12 at 14:15
  • \$\begingroup\$ @JerryCoffin: I see no down side to that. The overhead is during construction that happens once. Even if it is copied around a lot the extra cost of the copy is not that significant. And all this is totally outweighed by the usage of operator<< \$\endgroup\$ – Martin York Jul 1 '12 at 22:21
  • \$\begingroup\$ Is it really a reasonable assumption to say the copy is cheaper than the condition he previously had? Is there some way to quantify that? \$\endgroup\$ – David Jul 3 '12 at 13:03
  • \$\begingroup\$ @Dave. It is not cheaper it is more expensive. But you do not expect to see much copying (as I was trying to say). So I see the extra cost as insignificant, compared to the extra safety we achieve by doing it this way. \$\endgroup\$ – Martin York Jul 3 '12 at 15:26
  • \$\begingroup\$ I would leave delimiter and real_delim as charT const *, but in the constructor do as follows: infix_ostream_iterator(ostream_type &s, charT const *d = 0) : os(&s), delimiter(""), real_delimiter(d ? d : "") {}. That way there even less cost for copying. \$\endgroup\$ – dalle Jan 22 '14 at 7:30
15
\$\begingroup\$

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.

\$\endgroup\$
8
\$\begingroup\$

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.

\$\endgroup\$
5
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ Arguably, the type of d might be better as a template argument. I can envisage having it as a string literal (const char*) in some code, but as a std::string in others. \$\endgroup\$ – Toby Speight Mar 9 '17 at 9:10
4
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Maybe it's just that I'm not awake yet, but I guess I'm uncertain how this would work/be used. \$\endgroup\$ – Jerry Coffin Dec 11 '15 at 16:25
  • \$\begingroup\$ If you want a delimiter: make_ostream_iterator<T>(std::cout, ", ") and if you don't make_ostream_iterator<T>(std::cout). And then your object is only constructible with a delimiter. \$\endgroup\$ – Barry Dec 11 '15 at 16:27
  • \$\begingroup\$ The idea here is to be able to support something like copy(b, e, out), and have the elements of [b...e] come out as something like a, b, c, with delimiters between the items, but not after the last one (like std::ostream_iterator would produce). \$\endgroup\$ – Jerry Coffin Dec 11 '15 at 16:33
  • \$\begingroup\$ @JerryCoffin No, I know. But you're also supporting infix_ostream_iterator<T>{std::cout} with no delim and I'm suggesting you just don't. \$\endgroup\$ – Barry Dec 11 '15 at 16:40
  • \$\begingroup\$ Oh, I see--yeah, when you don't have a delimiter, there's no real difference between this and a normal ostream_iterartor. \$\endgroup\$ – Jerry Coffin Dec 11 '15 at 16:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.