Thursday, April 24, 2008

Q_FOREACH is your friend

It's real, foreach makes writing loops much more convenient, but you have to know how to use it, see the following code:


#include <QList>
#include <QtDebug>

class Foo
{
 public:
  Foo()
  {
   qDebug("Empty constructor");
  }

  Foo(int aa, int bb, int cc) : a(aa), b(bb), c(cc)
  {
   qDebug("Constructor");
  }

  Foo(const Foo &f)
  {
   a = f.a;
   b = f.b;
   c = f.c;
   qDebug("Copy constructor");
  }

  int a, b, c;
};

int main(int /*argc*/, char ** /*argv*/)
{
 QList<Foo> list;
 list << Foo(1, 2, 3);
 list << Foo(4, 5, 6);
 list << Foo(7, 8, 9);
 list << Foo(10, 11, 12);
 list << Foo(13, 14, 15);

 foreach(Foo f, list)
  f.a = f.b = f.c = 0;

 foreach(Foo f, list)
  qDebug() << f.a << f.b << f.c;
}


There are two problems in the usage of foreach in this code.

The first problem is serious, it is trying to use foreach to modify the values of vector. The code compiles, but if you run it, you'll notice nothing was changed, why oh why?! may you ask. Because Foo f is a local variable to the foreach, so what you are doing is modifying a variable that exists in this very same line and nothing more.

The second problem is that you are constructing far much more Foo objects than needed, if you look at the output the copy constructor is called each time in the foreach, so the correct way for the debug line would be

 foreach(const Foo &f, list)
  qDebug() << f.a << f.b << f.c;
}


So remember, always that your foreach iterator is not a basic value like an int, double or pointer, use const &, it will be faster and will protect you against the try to use foreach to modify values problem, because

 foreach(const Foo &f, list)
  f.a = f.b = f.c = 0;

will not compile.

5 comments:

  1. Or develop in Java and you won't have these problems (you'll have other ones though).

    ReplyDelete
  2. What you describe is just c++ standard behavior. If you want to modify the values of the list, you should do it this way:

    foreach(Foo& f, list)
    f.a = f.b = f.c = 0;

    Or you could use a list of pointers, instead.

    ReplyDelete
  3. @anonymous -- well, in the sense that you always get the equivalent of Foo&, I suppose you're right. But you still run into people trying to modify the list when they're using Java's new for-each iteration construct, and they run into basically the same issues. Sure, you don't have to worry about copying, but then you pay for that by pretty much never being able to copy things anyway (let's not even talk about Cloneable). It's a different language, it's got different approaches. Of course the problems will be somewhat different.

    And if you're a fan of iterators, Qt provides iterators that behave essentially like Java's anyway.

    ReplyDelete
  4. @anonymous: No thanks.

    @TheInvisible: Yeah, just that this code won't compile... But it was a good try

    ReplyDelete
  5. Beware that Qt's implementation of foreach constructs a copy of the container. So using a mutable reference instead of the constant reference to modify container items inside the loop only modifies the items of the copy of the container, and not the items of the container itself.

    Additionally copying the whole container might badly hit performance. If you want speed, don't use it.

    ReplyDelete