QVector one-line deep copy?

[JonB]

@Christian-Ehrlicher
Sorry, I have only just seen your reply. I did wonder about my finding.

OK, I need to repro, when I have a second. Just to save me: note that I wish to go myVec[3].member = value (yes, writing into original struct, not treating it as an immutable and re-assigning the whole struct), will that make any difference, so that the vector does not know to deep-copy on this statement?

In a less-likely possibility: the mutual-change I happened to notice was on the QString member, I don't suppose there should be any issue because of that?

[Christian Ehrlicher]

@JonB said in QVector one-line deep copy?:

will that make any difference, so that the vector does not know to deep-copy on this statement?

It should not since operator[] is non-const and forces a detach()

/edit:

struct s
{
    int one = 1;
    int two = 2;
};
int main(int argc, char **argv)
{
  QVector<s> myVec = {{1, 2}, {3, 4}};
  QVector<s> copy(myVec);
  copy[1].one = 5;
  qDebug() << myVec[1].one;
  qDebug() << copy[1].one;;
}

-->
3
5

[JonB]

@Christian-Ehrlicher

It should not since operator[] is non-const and forces a detach()

Ahhhh. What if I am not actually using a []? :) My changes to current will happen via a reference or a pointer, not an index, like:

for (MyStruct &cur : current)
    cur.member = newValue;

And I want that to change an element's struct-member-value in current without affecting the "copied" one in saved. Is that pushing me down a rabbit-hole here? :)

[Christian Ehrlicher]

@JonB said in QVector one-line deep copy?:

for (MyStruct &cur : current)

This will also force a detach.

/edit:

int main(int argc, char **argv)
{
  QVector<s> myVec = {{1, 2}, {3 ,4}};
  QVector<s> copy(myVec);
  
  for (auto &v : myVec)
    v.one = 42;
  qDebug() << myVec[0].one << myVec[1].one;
  qDebug() << copy[0].one << copy[1].one;;
}

-->
42 42
1 3

[JonB]

@Christian-Ehrlicher
Sigh :) OK, I'll have to produce a repro... Because code works with saved.append() but not with saved = current.

Hang on, one more. Same to you as to @J-Hilk previously. I can't use QVector<int> copy(myVec);. Copying is not done during constructor. current & saved are persistent member variables. It is called explicitly at various points. So it has to be something like saved = current. Does that make a difference to what you are telling me should work?

[Christian Ehrlicher]

@JonB said in QVector one-line deep copy?:

Does that make a difference to what you are telling me should work?

No, it does exactly the same, our version calls the copy operator, yours the operator=() but they do the same (if not it would be wrong by design ;) ). Just check it out with my little repro - it does not change anything.

[JonB]

@Christian-Ehrlicher
Hmm, now just wait a cotton-picking moment! @sierdzio appears to sort me out, after all!

QVector<MyStruct> current, saved;

// on its own, this does *not* work;
// when I alter something in `current` it gets changed in `saved`
saved = current; 
// as soon as I put this line next, it *does* work, I see changes when I later alter something in `current`
saved.detach();

So..... ?

[J.Hilk]

@JonB
obviously the const only works on construction 😉

that said a small test example:

struct MyStruct {
    int memberA;
    QString memberC;
    MyStruct() : memberA{0}, memberC{QLatin1String()}
    {}
};

#include <QDebug>
#include <vector>
int main (int argc, char *argv[])
{
    QVector<MyStruct> current{1};
    QVector<MyStruct> saved(current);
    std::vector<MyStruct> deepCopy;
    for(auto s : current)
        deepCopy.push_back(s);
    current.first().memberA = 10;

    qDebug() << current.first().memberA << saved.first().memberA << deepCopy.at(0).memberA;
}

results in
10 0 0

so it detaches automatically.

It doesn't for you?

[JonB]

@Christian-Ehrlicher , or @jsulm [or indeed any of you helpful experts, it's just that this is getting tricky...]
I wonder if you would care to help. Because I think we need your deep understanding of C++ to see if you can understand what is going on!

As I said, in my code it definitely does not work without an explicit detach(), and (I think) it will be to do with pointers.

Just to resume: if I break just prior to the assignment into the struct of an element in current I can see that element in both current & saved, and they are indeed showing the same address. After the assignment the address remains the same in both of them, hence my observed behaviour. There is no detachment!

If in the debugger I put a watch point on the element in question. I do that via current[13] & saved[13] (debugger won't allow .at(13)). If I do that, at that instant the addresses change --- i.e. detachment occurs, because of the []. (I think it is the current[13] which changes while saved[13] stays the same, if that's what you would expect.)

Like you, I don't think I saw that in a test program. However, my real code has a nasty wrinkle going on. And I'm wondering if it's all @jsulm's fault!

In https://forum.qt.io/topic/120395/return-pointer-to-member-in-const-method/8 he proposed the following code (you'd have to read the whole thread to understand why), which I now use:

const int *pointerToMember() const { return &member; }

int *pointerToMember()  { const MyClass *_this = this;  return const_cast<int*>(_this->pointerToMember()); } // Now compiler knows that you want to call const pointerToMember

In my code for the first overload, I am actually marching through the QVector current returning a reference to the found element as a pointer, or nullptr if not found.

The way I get the pointer to the element I need to change (in current) is via the second, "writeable" overload there.

Now then: I have a suspicion this is the cause? I am getting a writeable element in a (shared) QVector via a const_cast<> of a const method. Does this cheat by any chance disable/break the "detach-on-write" we are relying on to make sure element in current gets changed but not in saved. I have a feeling.... :)

[Christian Ehrlicher]

At least this could be a reason although I don't see your actual code to say it for sure.
I would have returned &member directly also in the non-const version - then this should not happen for sure.

[JonB]

@Christian-Ehrlicher
:) OK, here's the lookup code:

const Class::MyStruct *Class::find(int arg) const
{
    for (const MyStruct &ms : current)
        if (ms.arg== arg)
            return &ms;
    return nullptr;
}

Class::MyStruct *Class::find(int arg)
{
    const Class *_this = this;
    return const_cast<MyStruct *>(_this->find(arg));
}

QVector<MyStruct> current, saved;  // member variables
current.append(...);  // this can be called at various times
saved = current;  // this can be called at various times

MyStruct *ms = find(something);  // this will be found in current
if (ms != nullptr)
    ms->someMember = newValue;  // want to change in current, only

On that last line the element in current gets its content changed, but the element in saved continues to point to the same, now changed, element, with no "detachment".

It's difficult at present for me to break this up, as code presently relies elsewhere on these two definitions.

I am suggesting that the const_cast<> in the second overload is what "breaks" any detachment, is that possible? I'm thinking that it leads to the code/Qt thinking that no detachment is necessary for an element, because it thinks that must have already happened given the non-const return result, something like that?

If so, how would you like me to rewrite it? Bear in mind the whole of the referenced thread, in which I steadfastly refuse to return an index from my lookup find(), as that is so inefficient compared to a pointer :)

Finally, assuming it is @jsulm's solution to that thread which causes the problem, can I sue him? :-:

[Christian Ehrlicher]

Correct - this code kills the copy on write behavior since current is not detached in find() because it's a const operation there.

[JonB]

@Christian-Ehrlicher
And? :) That solves the mystery here (I can use @sierdzio's explicit detach() for now), but leaves me with the other thread still not correctly answered, now :( You didn't say if I could sue @jsulm there....

Thanks to you, and all, for answering. I didn't even recall when I raised this "deep copy" question that it was going via/could be related to the const_cast<> method.