QString::append() weirdness

[mattc]

Hello,
does anybody know why in the following code 's' contains trash upon creation, but becomes valid after creating another QString?

#include <QApplication>
#include <QDebug>
 
int main(int argc, char *argv[])
{
    QApplication app(argc, argv);  
 
    const QString& s = QString("hello").append("world");
    qDebug() << s; // s is invalid here (seen in the debugger)
 
    QString t = QString("hello").append("world");
    qDebug() << s; // s is ok (??????)
 
    return 0;
}

#include <QApplication>
#include <QDebug>

int main(int argc, char *argv[])
{
    QApplication app(argc, argv);  

    const QString& s = QString("hello").append("world");
    qDebug() << s; // s is invalid here (seen in the debugger)
        
    QString t = QString("hello").append("world");
    qDebug() << s; // s is ok (??????)

    return 0;
}

To copy to clipboard, switch view to plain text mode 

Qt 4.7
VS 2008 SP2
Vista

[SixDegrees]

s is a reference. You're trying to initialize it with a temporary variable. After the initialization, the temporary no longer exists and s has nothing to refer to.

It probably appears to contain something after the next initialization - which uses actual variables instead of references - because the memory region s was pointing at now just happens to contain the contents of t. Or because some other memory rearrangement takes place that makes it appear valid.

In short, you're not using references correctly here. References have to refer to something; they don't contain any values of their own.

Get rid of the ampersand and everything will work correctly, I'd bet.

[mattc]

AFAIK, binding a temporary to a const reference is perfectly legal in C++, I am doing that all the time.

I guess my problem has to do with the infamous "implicit sharing" of Qt data types. I think in this case it is breaking the C++ rules in a very dangerous way.

[Zlatomir]

AFAIK, binding a temporary to a const reference is perfectly legal in C++, I am doing that all the time...

I strongly suggest you stop doing that, assign a temporary value to a reference is a source of bugs (somewhat simmilar with using pointers after the delete call)

[faldzip]

AFAIK, binding a temporary to a const reference is perfectly legal in C++, I am doing that all the time.

I guess my problem has to do with the infamous "implicit sharing" of Qt data types. I think in this case it is breaking the C++ rules in a very dangerous way.

But did you checked that using non-Qt non-implicit shared class will give you result you expect?
I didn't know so I checked (it is not that hard) and I think that you're wrong.

Check this code:

#include <iostream>
 
class List
{
public:
    List() : head(0) {}
    ~List();
    char operator[](int index) const;
    List &prepend(char item);
 
private:
    struct Elem {
        char data;
        Elem *next;
    };
    Elem *head;
};
 
List::~List()
{
    for (Elem *e = head; e != 0;) {
        Elem *d = e;
        e = e->next;
        delete d;
    }
}
 
char List::operator[](int index) const
{
    Elem *tmp = head;
    for (int i = 0; i < index; ++i) {
        tmp = tmp->next;
    }
    return tmp->data;
}
 
List &List::prepend(char item)
{
    Elem *e = new Elem;
    e->data = item;
    e->next = head;
    head = e;
    return *this;
}
 
int main()
{
    const List &l = List().prepend('a');
    std::cout << l[0] << std::endl;
 
    List s;
    const List &t = s.prepend('b');
    std::cout << t[0] << std::endl;
 
    return 0;
}

#include <iostream>

class List
{
public:
    List() : head(0) {}
    ~List();
    char operator[](int index) const;
    List &prepend(char item);

private:
    struct Elem {
        char data;
        Elem *next;
    };
    Elem *head;
};

List::~List()
{
    for (Elem *e = head; e != 0;) {
        Elem *d = e;
        e = e->next;
        delete d;
    }
}

char List::operator[](int index) const
{
    Elem *tmp = head;
    for (int i = 0; i < index; ++i) {
        tmp = tmp->next;
    }
    return tmp->data;
}

List &List::prepend(char item)
{
    Elem *e = new Elem;
    e->data = item;
    e->next = head;
    head = e;
    return *this;
}

int main()
{
    const List &l = List().prepend('a');
    std::cout << l[0] << std::endl;

    List s;
    const List &t = s.prepend('b');
    std::cout << t[0] << std::endl;

    return 0;
}

To copy to clipboard, switch view to plain text mode 

There is no Qt class nor implicit sharing. It is simple list implementation, with prepend() method (the same idea as in append, but in my case prepend was easier to implement). What output you expect?

[wysota]

AFAIK, binding a temporary to a const reference is perfectly legal in C++, I am doing that all the time.

Are you sure your reference lives longer than the temporary variable? I strongely doubt that.

[Zlatomir]

Maybe the 'infamous "implicit sharing" ' makes some of the code to just "look like" it "works", but it still a bad practice and can anytime give you the world-famous "Segmentation Fault" error.

And another issue is that you can't modify a variable by using a const reference to it (even if the variable itself isn't const)

[wysota]

AFAIK, binding a temporary to a const reference is perfectly legal in C++, I am doing that all the time.

It is really easier to make a test than to ponder about things one "thinks" he knows. And so I made a test...

Here is our test subject:

#include <QtDebug>
 
class TestSubject {
public:
  TestSubject() {
   qDebug() << Q_FUNC_INFO;
  }
  ~TestSubject() {
   qDebug() << Q_FUNC_INFO;
  }
  TestSubject& operator=(const TestSubject &other) {
    qDebug() << Q_FUNC_INFO;
    return *this;
  }
 
  TestSubject(const TestSubject &other) {
    qDebug() << Q_FUNC_INFO;
  }
};

#include <QtDebug>

class TestSubject {
public:
  TestSubject() {
   qDebug() << Q_FUNC_INFO;
  }
  ~TestSubject() {
   qDebug() << Q_FUNC_INFO;
  }
  TestSubject& operator=(const TestSubject &other) {
    qDebug() << Q_FUNC_INFO;
    return *this;
  }

  TestSubject(const TestSubject &other) {
    qDebug() << Q_FUNC_INFO;
  }
};

To copy to clipboard, switch view to plain text mode 

Here is our test:

const TestSubject &testFun() {
  return TestSubject();     
}
 
int main() {
    const TestSubject &obj = testFun();
    qDebug() << "After testFun call";
    return 0;
}

const TestSubject &testFun() {
  return TestSubject();     
}

int main() {
    const TestSubject &obj = testFun();
    qDebug() << "After testFun call";
    return 0;
}

To copy to clipboard, switch view to plain text mode 

Here is the compilation log on gcc:

main.cpp: In function ‘const TestSubject& testFun()’:
main.cpp:20: warning: returning reference to temporary

And here is the output:

TestSubject::TestSubject() 
TestSubject::~TestSubject() 
After testFun call

TestSubject::TestSubject() 
TestSubject::~TestSubject() 
After testFun call

To copy to clipboard, switch view to plain text mode 

meaning that there is no copy upon returning a const reference hence when the temporary object is deleted (which happens before the qDebug() line in main()) the const reference points to an invalid object. Regardless if you use implicit sharing or not as it has completely nothing to do with the situation.

Conclusion: binding a temporary to a const reference is not legal in C++.

[mattc]

Oops, it was not my intention to start a flame-like thread.

I found this page by Herb Sutter about the "lifetime of const references bound to temp objects":
http://herbsutter.com/2008/01/01/got...portant-const/

@Wisota: your code is actually returning (not just binding) a reference to a temporary object, and that is invalid.

@Faldzip: you are right, your code has the same problem as the one I posted, and there is no implicit sharing involved. Still, it is not clear to me why it does not work. I see the destructor being called after the call to your prepend(), but I see no reason for that. I believe the list should happily bind to the const reference and live longer.

Thansk everybody, now I see the problem is unrelated to Qt.

[wysota]

Oops, it was not my intention to start a flame-like thread.

I found this page by Herb Sutter about the "lifetime of const references bound to temp objects":
http://herbsutter.com/2008/01/01/got...portant-const/

@Wisota: your code is actually returning (not just binding) a reference to a temporary object, and that is invalid.

Correct me if I'm wrong but in the blog you mention the function call actually returns a perfectly good copy of the temporary object. According to me this is a different situation than the one in your code. The scope of the two situations is different.

[mattc]

Reading the blog I understand the scope is the same, but I am not sure, so I posted the question on comp.lang.c++ (for anyone interested):

http://groups.google.com/group/comp....648e41ec70ee5#

[wysota]

Just a side note - if in my example I change the function signature to return a copy of an object, then indeed the destructor of the object gets delayed. The same happens here:

int main() {
    const TestSubject &obj = TestSubject();
    qDebug() << "After testFun call";
    return 0;
}

int main() {
    const TestSubject &obj = TestSubject();
    qDebug() << "After testFun call";
    return 0;
}

To copy to clipboard, switch view to plain text mode 

Your situation seems to be somehow different but I fail to see where. Maybe because your temporary object is not const.

Edit: yes, that's exactly the case:

TestSubject& TestSubject::x() { return *this; }
 
int main() {
    const TestSubject &obj = TestSubject().x();
    qDebug() << "After testFun call";
    return 0;
}

TestSubject& TestSubject::x() { return *this; }

int main() {
    const TestSubject &obj = TestSubject().x();
    qDebug() << "After testFun call";
    return 0;
}

To copy to clipboard, switch view to plain text mode 

gives again:

TestSubject::TestSubject() 
TestSubject::~TestSubject() 
After testFun call

TestSubject::TestSubject() 
TestSubject::~TestSubject() 
After testFun call

To copy to clipboard, switch view to plain text mode 

[mattc]

Thank you for your efforts Wysota.

It turns out the code does not work "because the standard says so". In other words, while this code is ok:

// bind a temporary *expression* to a const reference
const QString& s = QString("hello");

// bind a temporary *expression* to a const reference
const QString& s = QString("hello");

To copy to clipboard, switch view to plain text mode 

this one leaves a dangling reference:

// bind a temporary *object* to a const reference
const QString& s = QString("hello").append("");

// bind a temporary *object* to a const reference
const QString& s = QString("hello").append("");

To copy to clipboard, switch view to plain text mode 

I believe that from a purely "technical" point of view there is nothing wrong with the second snippet (a compiler could easily generate working code), but in fact it is not supposed to work.

[wysota]

Thankfully with the "imfamous implicit sharing" Qt offers you can happily do this:

QString s = QString("hello").append("");

QString s = QString("hello").append("");

To copy to clipboard, switch view to plain text mode 

[mattc]

LOL

You know, I am a bit nervous about "implicit sharing" since I reported this bug (read: feature):
http://bugreports.qt.nokia.com/browse/QTBUG-12941

In short, though QLinkedList offers the same "iteration" interface of a std::list (begin(), end()), the behaviour of the iterators it provides is not standard conforming.

For example, the standard says that a std::list::end() iterator should remain constant through the lifetime of a list. But calling QLinkedList::clear() can in fact change the value returned by QLinkedList::end().

So, when in my code I blindly replaced some std::lists with QLinkedLists I got crashes everywhere.

Nothing to pull your hair about, of course, but still annoying... and guess who's to blame for that? :-)

[wysota]

I fail to see how this can make you nervous about implicit sharing itself. As long as you don't call clear() on the list the behaviour will be as you expect it. And the behaviour of clear() is not related to implicit sharing itself but rather to limiting memory usage of empty lists (which happens to be implemented using implicit sharing).

[tbscope]

Besides that, changing the items while iterating over them is not safe.