1

So having

struct ResultStructure
{
    ResultStructure(const ResultStructure& other)
    {
        // copy code in here ? using memcpy ? how???  
    }
    ResultStructure& operator=(const ResultStructure& other)
    {
        if (this != &other) {
            // copy code in here ?
        }
        return *this
    }
    int length;
    char* ptr;
};

How to implement "copy constructor" and "assignment operator"? (sorry - I am C++ nube)

Update: sbi and others ask - why do I want to manually deal with raw memory? My answer is simple - In a students project I am part of now we use lots of C library's such as for example OpenCV OpenAL and FFmpeg and there are more to come. Currently using C++ we try to create a graph based direct show like cross platform library that would be helpful in live video broadcasting and processing. Our graph elements currently use char* and int pairs for data exchange. To cast data to subscribed elements we use raw memcpy now. I want to go further and make it possible for us to make our graph elements base C++ template. So that one graph element would be capable of of sharing current graph element data with other Graph elements and that data it shares would be a structure that would contain not one char* one int but any number of data fields and nearly any elements inside. So that is why I need to understand how to create a basic C++ structure that implements "copy constructor" and "assignment operator" for me to be capable to use new for us data casting algorithms like

void CastData(T item){
    for(size_t i = 0 ; i < FuncVec.size(); i++){
        T dataCopy = item;
        FuncVec[i](dataCopy);
    }
}

instead of currently used

void CastData(char * data, int length){
    for(size_t i = 0 ; i < FuncVec.size(); i++){
        char* dataCopy = new char[length];
        memcpy(dataCopy, data, length);
        FuncVec[i](dataCopy, length);
                    delete[] dataCopy;
    }
}
Rella
  • 65,003
  • 109
  • 363
  • 636
  • 2
    Terminology correction: there are "copy constructor" and "assignment operator". There is no "copy operator". There is nothing wrong with this name, though, it just that it isn't used, probably to avoid confusion with copy constructors. – Sergei Tachenov Jan 31 '11 at 07:16

7 Answers7

10

You might want to explain why you want to manually deal with raw memory. I haven't done this in a long time, it's what std::string and std::vector where designed for:

struct ResultStructure
{
    // nothing else needed

    std::string data; // or std::vector<char>
};

But if you really need to do this the hard way (is this homework?), then be advised that it is, at first, surprisingly hard to get this right. For example, a naive implementation of the assignment operator might be like this:

// DON'T TRY THIS AT HOME!!
ResultStructure& ResultStructure::operator=(const ResultStructure& rhs)
{
  delete[] ptr;                               // free old ressource
  ptr = new char[rhs.length];                 // allocate new resourse
  std::copy(rhs.ptr, rhs.ptr+rhs.length, ptr; // copy data
  length = rhs.length;
}

If someone accidentally assigns an object to itself (which might happen if all you have is two references and you don't suspect them to refer to the same object), then this will fail fatally.
Also, what if new throws an exception? (It might throw std::bad_alloc if memory is exhausted.) Then we have already deleted the old data and have not allocated new data. The pointer, however, still points at where the old data used to be (actually, I think this is implementation-defined, but I have yet to see an implementation that changes a ptr upon deletion), and the class' destructor (you knew that class would need a destructor, right?) would then attempt to delete a piece of data at an address where no data is allocated. That's Undefined Behavior. The best you can hope for is that it crashes immediately.

The easiest way to do this is to employ the Copy-And-Swap idiom:

struct ResultStructure
{
    ResultStructure(const ResultStructure& other)
     : ptr(new char[rhs.length]), length(rhs.length)
    {
        std::copy(rhs.ptr, rhs.ptr+rhs.length, ptr);
    }

    ~ResultStructure() // your class needs this
    {
      delete[] ptr;
    }

    ResultStructure& operator=(ResultStructure rhs) // note: passed by copy
    {
        this->swap(rhs);
        return *this
    }

    void swap(const ResultStruct& rhs)
    {
      using std::swap;
      swap(length, rhs.length); 
      swap(ptr, rhs.ptr); 
    }

    std::size_t length;
    char* ptr;
};

Note that I have added a destructor, changed the assignment operator to pass the argument per copy (we need the copy constructor invoked to allocate memory), and added a swap member function. Per convention a swap() function never throws and is fast, preferably O(1).

I know that GMan's discussion of the Copy-And-Swap idiom is exhaustive enough to be and exhausting while mine is probably too terse for you, and you will likely not understand a lot of it at first, but try to persevere and to understand as much as possible.

Community
  • 1
  • 1
sbi
  • 219,715
  • 46
  • 258
  • 445
9

If you use std::string, instead of char*, you would not even need to write operator= or copy-constructor. The compiler generated code would do your job very well.

But as a general solution (for some other scenario), use copy-and-swap idiom:

Exceptional C++ by Herb Sutter has described these in great detail. I would recommend you to read items from this book. For the time being, you can read this article online:

Community
  • 1
  • 1
Nawaz
  • 353,942
  • 115
  • 666
  • 851
  • I'm afraid for someone asking a question like this, Sutter's _Exceptional C++_, while a very good book, might still be, um, a bit out of reach. I'd rather recommend to start with Meyers' _Effective C++_ first. The [newest edition](http://www.amazon.com/gp/product/0321334876) has an very good treatment about this in item 11, "Handle assignment to self in `operator=`". – sbi Jan 31 '11 at 09:32
4

The easy solution is to use a std::string instead of char* member.

Then the compiler-generated copy constructor and copy assignment operator just work.

As a rule, and especially as a novice, don't have raw pointer members.

Cheers & hth.,

Cheers and hth. - Alf
  • 142,714
  • 15
  • 209
  • 331
  • 2
    +1 *As a rule, and specially as a novice, don't have raw pointer members.* This is one of the best advices that you can get while starting in the languages. I would upvote twice if possible. – David Rodríguez - dribeas Jan 31 '11 at 08:40
2

As has been said, and as was recommending in the question this emanated from, you should probably reuse an existing container. At least the code would be right :)

For educational purposes though, let's examine this structure:

class Result
{
public:

private:
  size_t length; // can't really be negative, right ?
  char* data;
};

Here, we need explicit memory management. This implies, notably, following the Rule Of Three (say thanks to Fred)

Let's begin with actually building our object:

Result::Result(): length(0), data(0) {}

Result::Result(size_t l, char* d): length(0), data(0)
{
  if (!d) { return; }

  data = new char[l]; // this may throw, so we do it first

  length = l;
  memcpy(data, d, l);
}

Now we can implement the traditional operators:

// Swap
void Result::swap(Result& other)
{
  using std::swap;

  swap(length, other.length);
  swap(data, other.data);
}

// Copy Constructor
Result::Result(Result const& other): length(0), data(0)
{
  if (!other.length) { return; }

  data = new char[other.length];
  length = other.length;
  mempcy(data, other.data, length);
}

// Assignemt Operator
Result& Result::operator=(Result other)
{
  this->swap(other);
  return *this;
}

// !IMPORTANT!
// Destructor
Result::~Result()
{
  delete[] data; // use the array form of delete
                 // no need to test for nullity, it's handled
}
Community
  • 1
  • 1
Matthieu M.
  • 287,565
  • 48
  • 449
  • 722
  • It's `std::size_t`! `:)` Anyway, `+1` from me, because this is correct (although it mostly duplicates mine). – sbi Jan 31 '11 at 08:46
  • @sbi: didn't see your solution :) I've switched to class, if you'll remark, because there is a required invariant here that a `struct` make harder to maintain. – Matthieu M. Jan 31 '11 at 09:13
  • Yeah, that (and a few other details) are why I wrote "mostly". Had you been in the [chat](http://chat.stackoverflow.com/transcript/message/290443#290443), you would have known of my reply. `:)` What can I do to lure you there? – sbi Jan 31 '11 at 09:28
  • what functions should be private what public? – Rella Jan 31 '11 at 15:32
  • @Kabumbus: normally, they'll all be public. That said, you may wish for a type not to be constructible / destructible other than by its children (useful property for base classes) etc... but that's an edge case. – Matthieu M. Jan 31 '11 at 15:36
  • there are errors in your code... for example see http://codepad.org/Mn7m5GW1 error in memcpy(d, d + l, data); . Thenthere is C3861 on mempcy(other.data, other.data + length, data); – Rella Jan 31 '11 at 16:11
  • @Kabumbus: that should be better now :) I never use the C methods because it's so unnecessary in C++. – Matthieu M. Jan 31 '11 at 16:17
1

this is std::vector<char>'s job - or is this homework?

the vector would replace both length and the allocation behind ptr. the vector is the c++ idiom, and you'll not make friends with other c++ devs if you implement your classes like you've described. of course, there are corner cases, but standard containers such as vector are the default.

the vector knows how to copy chars as well as itself, and the implementations are optimized and tested.

here's how to explicitly implement copy ctor/assign using a vector:

struct ResultStructure {
    ResultStructure(const ResultStructure& other) : d_chars(other.d_chars) {
    }

    ResultStructure& operator=(const ResultStructure& other) {
        if (this != &other) {
            this->d_chars = other.d_chars;
        }
        return *this;
    }
    std::vector<char> d_chars;
};
justin
  • 104,054
  • 14
  • 179
  • 226
  • 1
    `std::string` would probably be even better here. Also, prefer the copy-and-swap idiom rather than the `if (this != &other)` trick. – Matthieu M. Jan 31 '11 at 08:18
  • @Matthieu M. perhaps `std::string` would be better. since the OP didn't specify whether length+ptr represented data or a string, i assumed data. regarding keeping the OP's `if (this != &other)`: i only implemented what was requested by the OP. – justin Jan 31 '11 at 08:28
  • @Justin: I know that is what the OP requested (and thus no downvote), but I prefer to educate people toward better idioms whenever possible :) The copy and swap reuse the copy constructor's code and thus favor eliminating redundancy (though honestly, there's still too much for my taste... C++ is verbose :/) – Matthieu M. Jan 31 '11 at 08:40
  • 1
    @Justin: Once you use `std::vector`, neither the copy constructor nor the assignment operator are necessary. The compiler-generated implementations would suffice. – sbi Jan 31 '11 at 08:45
  • 1
    @sbi yes, *i* know that std::vector implements this and the compiler generates these correctly - i was just answering the question without getting too carried away in detail (there's really many more details which could be covered). at any rate, +1 because the point is likely to help somebody who doesn't know. – justin Jan 31 '11 at 09:07
-1

How is the memory to which ptr points allocated?

if using new, allocate with new, set length and then copy

other.length = length;
other.ptr = new char[length];
memcpy( other.ptr, ptr, length );

If you're allocating the memory with malloc, substitute a call to malloc in place of the call to new. If you're using some other memory scheme, figure it out. :)

JimR
  • 15,513
  • 2
  • 20
  • 26
  • 2
    -1 the code is incorrect (as I'm writing this). also, when advocating `new` and raw pointers one should as minimum discuss rule of three. – Cheers and hth. - Alf Jan 31 '11 at 07:45
  • @Alf P. Steinbach: Read closer, I'm advocating nothing. Simply trying to get him to understand his question has a subjective area (memory allocation) without cluttering everything with every possible pedantic argument. If is a big word. – JimR Jan 31 '11 at 10:54
  • @Kabumbus: this is a pointer. In the codepad.org code you linked, you're using a `.` to access local class members. Use the pointer notation, which is `->` as in `this->length` or `this->ptr`. As Alf points out, this is not perfect code. I was giving you fish instead of attempting to teach you how to fish. On this site, I guess that could be seen as a failure on my part. Such is life :). If you're going to be doing C++ programming as a profession, you should learn some of the things pointed out to you by Alf, Nawaz and sbi among others. – JimR Jan 31 '11 at 11:26
-1

I think this should do the work:

struct ResultStructure
{
    ResultStructure(const ResultStructure& other);
    ResultStructure& operator=(const ResultStructure& other);

    int length;
    char* ptr;
};

ResultStructure::ResultStructure(const ResultStructure& other):length(other.length)
{
    ptr = (char*)malloc(length);
    memcpy(ptr, other.ptr, length);
}

ResultStructure& ResultStructure::operator=(const ResultStructure& other)
{
    length = other.length;
    ptr = (char*)malloc(length);
    memcpy(ptr, other.ptr, length);
    return *this;
}

Please remember about freeing ptr in destructor. What is stored under ptr? If text, why not to use std::string? Anyway you can use std::vector. The constructors will be much easier then...

justin
  • 104,054
  • 14
  • 179
  • 226
Patryk
  • 1,421
  • 8
  • 21
  • 2
    You're not using initialization lists (bad style), advocate the use of `malloc()` in C++ (bad code), and your assignment operator doesn't free the old memory (wrong code). That's a very definitive `-1` from me. – sbi Jan 31 '11 at 08:06
  • @Kabumbus: If you don't know C++ good enough to accept a good answer (or at the very least one that isn't as fatally wrong as this one), you might want to use the voting as an orientation. Nawaz' answer is very good. If you don't understand it, then _ask_ about it. – sbi Jan 31 '11 at 08:08
  • 1
    @sbi Nawaz answers are always very good but often with no code samples. – Rella Jan 31 '11 at 08:17
  • @Kabumbus: I understand. I have posted an answer which, hopefully, is elaborated enough for you to understand a few of the issues involved. – sbi Jan 31 '11 at 08:24
  • 1
    @Kabumbus note that your implementation which 'compiles correctly' fixes none of the issues pointed out by sbi. fwiw, the points sbi made are all worthy of rejection from my codebases as they do not conform to their standards. in fact, there are a few other issues - the most prominent being the use of c-style casting. – justin Jan 31 '11 at 08:39