Object being copied in to a vector being cleared because original pre-copy is clear()ed

Question

I have something similar to:

#include<vector>

using namespace std;

vector<char> temp;
vector<char> allbytes = GetBytes();
vector<MyClass> outsidecontainer;

for(int i=0; i<allbytes.size(); i++){

    //Populate my buffer
    if(something){
        temp.push_back(allbytes[i]);
    }

    //temporary buffer now needs to be used to create MyClass object 
    //and outside container store this MyClass object
    else{
        MyClass m(temp);
        outsidecontainer.push_back(m);

        //Empty the temporary buffer ready for next population
        temp.clear();
    }
}

class MyClass{
    public:
    MyClass(vector<char> Message);

    private:
    vector<char> Message;
};

The problem is that come the end, outsidecontainer holds empty MyClass objects. In other words, temp has been emptied due to clear(). However, I didnt think this would affect the value in the outsidecontainer because temp is copied in to MyClass m, which is also copied in to outsidecontainer. They are not storing reference or pointers to objects??

How can I implement the above design, being able to use temp to create MyClass objects and clear it for the next population?

EDIT:

Even though MyClass m has loop-scope, does the object added to outsidercontainer remain after the loop has finished because the value was copied in to the array?

EDIT2:

#include "FIXMessage.h"


FIXMessage::FIXMessage(vector<char> message){
    Message = message;
}

FIXMessage::FIXMessage(const FIXMessage& rhs){

}

What does `MyClass` look like? Is it holding the `char`s by pointer or reference? — abarnert, Sep 21 '13 at 00:36
You need to show us a [complete example](http://sscce.org). When I add some bare minimum extra code to this to make it something I can run and test (like [this](http://pastebin.com/2jV2cZGr)), I don't end up with empty objects. So the problem must be somewhere in the code you haven't shown us. — abarnert, Sep 21 '13 at 01:32
@abarnert Could it be because of my copy constructor (see edit)? — user997112, Sep 21 '13 at 01:50
Guys- it was because I didn't implement my copy constructor. I needed to assign rhs.message to the "this" message. — user997112, Sep 21 '13 at 01:54
@user997112: You don't need to "assign rhs.message to the this message". If your class look like what you posted, you have to get rid of your version of copy constructor entirely. The compiler will generate the proper copy constructor by itself and it will do a better job than what you did. — AnT stands with Russia, Sep 21 '13 at 04:58

AnT stands with Russia · Answer 1 · 2013-09-21T01:59:41.007

2

Your second edit shows this

FIXMessage::FIXMessage(const FIXMessage& rhs){
}

By doing this you explicitly forced FIXMessage copy constructor to copy nothing. This is why all objects you copy with this copy constructor end up empty. And yes, copy constructor is used when adding elements to a vector.

If you want to write your own copy constructor, it becomes your responsibility to carefully copy all subobjects of the class: base subobjects and member subobjects. You have to do it manually. Instead, you completely suppressed all copying. Why?

No wonder the copied objects end up empty.

But the real question here is whether you actually need a manually implemented copy constructor. Maybe the compiler provided one will work fine? There's no way to say without seeing what your FIXMessage class contains.

For example, if your FIXMessage contains a std::vector and nothing else, the you don't have to write the copy constructor at all. The compiler will provide one for you, which will copy everything correctly.

Finally, learn to use references and initializer lists

FIXMessage::FIXMessage(const vector<char> &message) : Message(message) 
  {}

Passing heavy objects, like std::vector, by value doesn't make much sense, unless you have a very good reason to do so.

P.S. So the original code you posted was fake. Don't post fake code. It can only waste people's time.

edited Sep 21 '13 at 01:59

answered Sep 21 '13 at 01:54

AnT stands with Russia

312,472
42
525
765

Good overall, but "Passing heavy objects, like std::vector, by value" is usually not a problem unless you can't use C++11. In his case, his version doesn't means two copy constructor calls, it just means a copy and a move, which is generally cheap (that being the whole point of move constructors). And using references everywhere to avoid copies that don't exist may be harmless (even a tiny bit helpful) here, but there are many cases where it actually makes things worse, so following it blindly is not good advice. – abarnert Sep 23 '13 at 18:03
@abarnert: I don't see how it could be a copy and a move. The function parameter is not a temporary, meaning that it has to be copied from the argument and it has to be copied to the object data member. It is two copies. The user can explicitly request a move from the parameter to the data memebr through `std::move`, but this is a different story. In general, I don't see any reason to pass heavy objects by value unless you really need the parameter to be a copy. – AnT stands with Russia Sep 23 '13 at 18:53
[This question](http://stackoverflow.com/questions/10231349/are-the-days-of-passing-const-stdstring-as-a-parameter-over) has a pretty good discussion, and there are lots of Related questions and outside links. If you understand the issues described there, it's relatively easy to make the right choice in each case. If you just follow the dogma "always pass by reference", you're going to make the wrong choice half the time. – abarnert Sep 23 '13 at 19:08
@abarnert: That is completely incorrect. Function parameter is an *lvalue*. Where did you get the stange idea that it is an *rvalue* is compleytely unclear to me. There's no difference between function parameter and an ordinary local variable. Both are lvalues. – AnT stands with Russia Sep 23 '13 at 19:15
@abarnert: Even though your link is broken, I know what article you are talking about. The article states very clearly: when you definitely need a local copy (and only when you need a local copy) it is better to pass by value (i.e. let the compiler to perform the copying) instead of passing by reference and making the local copy manually. That is the whole point of the article. Again, "when you need a copy, let the compiler do it" - that's the whole point. But when you don't need a copy, the article simply does not apply. – AnT stands with Russia Sep 23 '13 at 19:16
@abarnert: I don't know why so many people misinterpret that article into "you can/should always pass by value". This is completely incorrect and the article does even attempt to say anything like that. The SO discussion you linked yourself is pretty clear on that, so I suggest you read it. In our example here we don't need a local copy, which is why the article does ot apply at all. In our example here passing by reference is the perfect thing to do. – AnT stands with Russia Sep 23 '13 at 19:20
Of course the article doesn't say "you should always pass by value". It says you should _sometimes_ pass by value. And sometimes by reference. And you have to actually understand when you should pass by value, and when by reference. And it's not that hard. Implying you should always pass by reference is just as wrong as implying you should always pass by value. – abarnert Sep 23 '13 at 19:23
@abarnert: I never said that one "should always pass by reference". The article you linked is dedicated to a very specific situation: one wants to modify the parameter inside the function and one wants the actual argument to remain unchanged. I.e. one definitely needs a local copy. What is the best way to create that copy? Pass by value? Or pass by reference and make the copy manually inside? The article says: pass by value is generally more efficient. I agree with the reasoning in that article. But that has absolutely nothing to with our situation here. We simply don't need a local copy here. – AnT stands with Russia Sep 23 '13 at 19:27
...and when we *don't* need a local copy, pass-by-const-reference remains an unchallenged king for heavy objects. – AnT stands with Russia Sep 23 '13 at 19:29
Yes, but your "learn to use references" is going to sound a lot more like "always pass by const reference" to a novice than "pass by const reference if the object is heavy and you don't need a local copy". – abarnert Sep 23 '13 at 19:50

score 0 · Answer 2 · answered Sep 21 '13 at 01:48

I might be wrong as we dont know what's going on in your MyClass constructor, but your constructor parameter and vector member share the same name... This is probably the problem.

I just gave a try to your code with this MyClass :

class MyClass{
    public:
    MyClass(vector<char> Message)
    {
        m_Message = Message;
    }

    private:
    vector<char> m_Message;
};

and the outsidecontainer vector still contains all values entered at the end of the loop i.e after many temp.clear();

Hope this helps

Object being copied in to a vector being cleared because original pre-copy is clear()ed

2 Answers2