Is this use of the "," operator considered bad form?

Question

I have made a list class as a means of replacing variadic functions in my program used for initializing objects that need to contain a changing list of elements. The list class has a usage syntax that I really like. However I haven't seen it used before, so I was wondering if I shouldn't use it just because of that fact? A basic implementation of the list class looks like this...

#include <list>
#include <iostream>
template<typename T>
struct list
{
    std::list<T> items;
    list(const list&ref):items(ref.items){}
    list(){}
    list(T var){items.push_back(var);}
    list& operator,(list add_){
        items.insert(items.end(),add_.items.begin(), add_.items.end());
        return *this;
    }
    list& operator=(list add_){
        items.clear();
        items.insert(items.end(),add_.items.begin(), add_.items.end());
        return *this;
    }
    list& operator+=(list add_){
        items.insert(items.end(),add_.items.begin(), add_.items.end());
        return *this;
    }
};

This allows me to have use this in code like so...

struct music{
//...
};
struct music_playlist{
    list<music> queue;
//...
};
int main (int argc, const char * argv[])
{
    music_playlist playlist;
    music song1;
    music song2;
    music song3;
    music song4;
    playlist.queue = song1,song2; // The queue now contains song1 and song2
    playlist.queue+= song1,song3,song4; //The queue now contains two song1s and song2-4
    playlist.queue = song2; //the queue now only contains song2
    return 0;
}

I really think that the syntax is much nicer than it would of been if I had just exposed a regular stl container, and even nicer (and typesafe) than variadic functions. However, since I have not seen this syntax used, I am curious about whether I should avoid it, because above all the code should be easily understood by other programmers?

EDIT:

In joint with this question, I have posted this question more targeted at solutions to the actual problem.

Answer 1

Why not overload the << operator as QList does? Then use it like:

playlist.queue << song1 << song2; // The queue now contains song1 and song2
playlist.queue << song1 << song3 << song4; //The queue now contains two song1s and song2-4

Answer 2

I agree that your syntax looks nice as you have written it.
My main difficulty with the code is that I would expect the following to be the same

playlist.queue = song1,song2;
playlist.queue = (song1,song2);  //more of c-style, as @Iuser notes.

whereas in fact they are completely different.

This is dangerous because its too easy to introduce usage bugs into the code. If someone likes to use parenthesis to add extra emphasis to groupings (not uncommon) then the comma could become a real pain. For example,

//lets combine differnt playlists
new_playlist.queue =    song1        //the first playlist
                      ,(song3,song4) //the second playlist //opps, I didn't add song 3!
                      , song5;        //the third 

or

new_playlist.queue = (old_playlist.queue, song6); //opps, I edited my old playlist too!

Incidently, have you come across boost.assign: http://www.boost.org/doc/libs/1_47_0/libs/assign/doc/index.html

Answer 3

Has the precedence changed recently?

playlist.queue = song1,song2;

This should parse as:

(playlist.queue = song1) , song2;

Your ',' and '+=' are the same! It would be a better semantic match if your comma operator were to create a temporary list, insert the left and right items and return the temporary. Then you could write it like this;

playlist.queue = (song1,song2);

with explicit parens. That would give C-programmers a fighting chance at being able to read the code.

Answer 4

A bit of a problem is that if the compiler cannot choose your overloaded operator comma, it can fall back on using the built-in operator.

In contrast, with Boost.Assign mixing up types produces a compilation error.

#include <boost/assign.hpp>

int main()
{
    int one = 1;
    const char* two = "2";
    list<int> li;
    li = one, two;

    using namespace boost::assign;
    std::list<int> li2;
    li2 += one, two;
}

Answer 5

This is probably something that belongs over on Programmers, but here's my two cents.

If you're talking about code that has a fairly narrow context, where users will use it in a couple of places and that's all, then overloading the , operator is probably OK. If you're building a domain-specific language that is used in a particular domain and nowhere else, it's probably fine.

The issue comes when you're overloading it for something that you expect the user to use with some frequency.

Overloading , means that the reader needs to completely reinterpret how they read your code. They can't just look at an expression and instantly know what it does. You're messing with some of the most basic assumptions that C++ programmers make when it comes to scanning code.

Do that at your own peril.

Answer 6

I am curious about whether I should avoid it, because above all the code should be easily understood by other programmers

If the goal is to make your code easy for other C++ programmers to understand, overriding operators to give them a meaning that's very different from that of standard C++ is not a good start. Readers shouldn't have to a) understand how you've implemented your container and b) recalibrate their understanding of standard operators just to be able to make sense of your code.

I can appreciate the Boost precedent for this sort of thing. If you're pretty sure that most of the people who will read your code will also be familiar with Boost Assign, your own override of operator, might be pretty reasonable. Still, I'd suggest following @badzeppelin's suggestion to use operator<< instead, just as iostreams does. Every C++ developer can be counted on to have run into code like:

cout << "Hello world!"`

and your append operation is very similar to writing to a stream.

Answer 7

It's bad on so many levels...

You're overriding list and shadowingstd::list. A big no-no. If you want your own list class - make it be with a different name, don't shadow the standard library.

Using , in such way is not readable. The return value of the operator is the right operand. Even if your code works, for an external reader it won't be obvious why, and it's a bad thing. Code should be readable, not nice.

Answer 8

There is nothing bad about using comma operator , using specifically. Any operator leaves bad taste, if exploited. In your code, I don't see any reasonable problem. Only one suggestion, I would like to give is:

list& operator,(list &add_){  // <--- pass by reference to avoid copies
  *this += add_;  // <--- reuse operator +=
  return *this;
}

This way, you have to always edit just operator +=, if you want any change in logic. Note that, my answer is in the perspective of readability and code maintenance in general. I will not raise concern about business logic you use.