Overloaded assignment

Good evening.

Code:

 
  thames@semaht ~/C++/Projects/Disks $ g++ -g -Wall maindisk1.cpp disk1.cpp -o maindisk1 -std=c++11
thames@semaht ~/C++/Projects/Disks $ valgrind ./maindisk1
==2721== Memcheck, a memory error detector
==2721== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
==2721== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info
==2721== Command: ./maindisk1
==2721== 
Using object directly:
Label: Capitol
Performers: Beatles
Number of selections: 14
Playtime: 35.50
Primary work: Piano Sonata in B flat, Fantasia in C
Label: Philips
Performers: Alfred Brendel
Number of selections: 2
Playtime: 57.17
Using type cd * pointer to objects:
Label: Capitol
Performers: Beatles
Number of selections: 14
Playtime: 35.50
Primary work: Piano Sonata in B flat, Fantasia in C
Label: Philips
Performers: Alfred Brendel
Number of selections: 2
Playtime: 57.17
Calling a function with a Cd reference argument:
Label: Capitol
Performers: Beatles
Number of selections: 14
Playtime: 35.50
Primary work: Piano Sonata in B flat, Fantasia in C
Label: Philips
Performers: Alfred Brendel
Number of selections: 2
Playtime: 57.17
Testing assignment: 
==2721== Conditional jump or move depends on uninitialised value(s)
==2721==    at 0x401292: Cd::operator=(Cd const&) (disk1.cpp:72)
==2721==    by 0x40163C: Classic::operator=(Classic const&) (disk1.cpp:117)
==2721==    by 0x400DA3: main (maindisk1.cpp:27)
==2721== 
==2721== Conditional jump or move depends on uninitialised value(s)
==2721==    at 0x4C2A07A: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2721==    by 0x4012A3: Cd::operator=(Cd const&) (disk1.cpp:72)
==2721==    by 0x40163C: Classic::operator=(Classic const&) (disk1.cpp:117)
==2721==    by 0x400DA3: main (maindisk1.cpp:27)
==2721== 
==2721== Invalid free() / delete / delete[] / realloc()
==2721==    at 0x4C2A0C7: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2721==    by 0x4012A3: Cd::operator=(Cd const&) (disk1.cpp:72)
==2721==    by 0x40163C: Classic::operator=(Classic const&) (disk1.cpp:117)
==2721==    by 0x400DA3: main (maindisk1.cpp:27)
==2721==  Address 0x40189d is in the Text segment of /home/thames/C++/Projects/Disks/maindisk1
==2721== 
==2721== Conditional jump or move depends on uninitialised value(s)
==2721==    at 0x4012F1: Cd::operator=(Cd const&) (disk1.cpp:75)
==2721==    by 0x40163C: Classic::operator=(Classic const&) (disk1.cpp:117)
==2721==    by 0x400DA3: main (maindisk1.cpp:27)
==2721== 
==2721== Conditional jump or move depends on uninitialised value(s)
==2721==    at 0x4C2A07A: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2721==    by 0x401302: Cd::operator=(Cd const&) (disk1.cpp:75)
==2721==    by 0x40163C: Classic::operator=(Classic const&) (disk1.cpp:117)
==2721==    by 0x400DA3: main (maindisk1.cpp:27)
==2721== 
==2721== Invalid free() / delete / delete[] / realloc()
==2721==    at 0x4C2A0C7: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2721==    by 0x401302: Cd::operator=(Cd const&) (disk1.cpp:75)
==2721==    by 0x40163C: Classic::operator=(Classic const&) (disk1.cpp:117)
==2721==    by 0x400DA3: main (maindisk1.cpp:27)
==2721==  Address 0x4e3c998 is not stack'd, malloc'd or (recently) free'd
==2721== 
==2721== Conditional jump or move depends on uninitialised value(s)
==2721==    at 0x401648: Classic::operator=(Classic const&) (disk1.cpp:118)
==2721==    by 0x400DA3: main (maindisk1.cpp:27)
==2721== 
==2721== Conditional jump or move depends on uninitialised value(s)
==2721==    at 0x4C2A07A: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2721==    by 0x401659: Classic::operator=(Classic const&) (disk1.cpp:118)
==2721==    by 0x400DA3: main (maindisk1.cpp:27)
==2721== 
==2721== Invalid free() / delete / delete[] / realloc()
==2721==    at 0x4C2A0C7: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2721==    by 0x401659: Classic::operator=(Classic const&) (disk1.cpp:118)
==2721==    by 0x400DA3: main (maindisk1.cpp:27)
==2721==  Address 0x400b30 is in the Text segment of /home/thames/C++/Projects/Disks/maindisk1
==2721== 
Primary work: Piano Sonata in B flat, Fantasia in C
Label: Philips
Performers: Alfred Brendel
Number of selections: 2
Playtime: 57.17
==2721== 
==2721== HEAP SUMMARY:
==2721==     in use at exit: 0 bytes in 0 blocks
==2721==   total heap usage: 8 allocs, 11 frees, 138 bytes allocated
==2721== 
==2721== All heap blocks were freed -- no leaks are possible
==2721== 
==2721== For counts of detected and suppressed errors, rerun with: -v
==2721== Use --track-origins=yes to see where uninitialised values come from
==2721== ERROR SUMMARY: 9 errors from 9 contexts (suppressed: 2 from 2)

Code:

 
#ifndef DISK1_H_
#define DISK1_H_

// base class
class Cd  // represents a CD disk
{ 
  private:
    char* performers;
    char* label;
    int selections;
    // number of selections
    double playtime; // playing time in minutes
  public:
    Cd(){}
    Cd(const char* pfs, const char* lbl, const int sls, const double pte);
    Cd(const Cd & c);
    virtual ~Cd();
    virtual void Report() const; // reports all CD data
    Cd& operator=(const Cd & c);
};

class Classic : public Cd
{  
   private: 
     char* primary_work;
   public: 
    Classic() {} 
    Classic(const char* p_work, const char* pfs, 
     const char* lbl, const int sls,const double pte);
     Classic(const char* p_work, const Cd& c);       
     Classic(const Classic& cl);
     virtual ~Classic();
     virtual void Report() const;
     Classic& operator=(const Classic& cl);
};     
#endif

Code:

 
// implements interface disk1.h 
#include "Headers/disk1.h"
#include <iostream> 
#include <cstring>
#include <cstdlib>

using std::cout;
using std::strlen; 
using std::strcpy;
using std::ios_base;
using std::endl;

typedef std::ios_base::fmtflags Format; 
typedef std::streamsize Precis;

Cd::Cd(const char* pfs, const char* lbl, const int sls, const double pte)
{ 
   performers = new char[strlen(pfs) + 1];
   strcpy(performers, pfs); 
   label = new char[strlen(lbl) + 1];
   strcpy(label, lbl);
   if(sls > 0 && sls <= 20) 
     selections = sls; 
   else 
   {  
     cout << "Selections must be a value between 1 and 20";
     exit(EXIT_FAILURE);      
   }  
   if(pte > 2.0 && pte < 60.0)
     playtime = pte; 
   else 
   {  
     cout << "Playtime must be a value between 2 and 60 with fraction point";     
     exit(EXIT_FAILURE);
   }
}     

Cd::Cd(const Cd& c) 
{ 
   performers = new char[strlen(c.performers) + 1];
   strcpy(performers, c.performers);
   label = new char[strlen(c.label) + 1];
   strcpy(label, c.label);
   selections = c.selections; 
   playtime = c.playtime;     
}      

Cd::~Cd() 
{ 
   delete[] performers; 
   delete[] label;    
}     

void Cd::Report() const 
{ 
   Format initial = cout.setf(ios_base::fixed, ios_base::floatfield);
   Precis prec = cout.precision(2);
   
   cout << "Label: " << label << endl;
   cout << "Performers: " << performers << endl; 
   cout << "Number of selections: " <<     selections << endl;
   cout << "Playtime: " << playtime << endl; 
   
   cout.setf(initial); 
   cout.precision(prec);
}     

Cd& Cd::operator=(const Cd& c)
{ 
    if(this == &c) 
      return *this; 
    delete[] performers; 
    performers = new char[strlen(c.performers) + 1];
    strcpy(performers, c.performers);
    delete[] label; 
    label = new char[strlen(c.label) + 1];
    strcpy(label, c.label);
    selections = c.selections; 
    playtime = c.playtime; 
    return *this;       
}     

Classic::Classic(const char* p_work, const char* pfs, const char* lbl, 
const int sls, const double pte) : Cd(pfs, lbl, sls, pte)
{ 
   primary_work = new char[strlen(p_work) + 1];
   strcpy(primary_work, p_work); 
}     

Classic::Classic(const char* p_work, const Cd& c) : Cd(c)
{ 
   primary_work = new char[strlen(p_work) + 1]; 
   strcpy(primary_work, p_work);
}

Classic::Classic(const Classic& cl) : Cd(cl)
{ 
   primary_work = new char[strlen(cl.primary_work) + 1]; 
   strcpy(primary_work, cl.primary_work);    
}     

Classic::~Classic() 
{ 
  delete[] primary_work;     
}     

void Classic::Report() const 
{ 
   cout << "Primary work: " << primary_work << endl; 
   Cd::Report();
}          

Classic& Classic::operator=(const Classic& cl)
{ 
   if(this == &cl) 
    return *this; 
   Cd::operator=(cl);
   delete [] primary_work; 
   primary_work = new char[strlen(cl.primary_work) + 1];
   strcpy(primary_work, cl.primary_work);
   return *this;     
}

Code:

 
// main program -- compile with disk1.cpp
#include <iostream>
using namespace std;
#include "Headers/disk1.h"

void Bravo(const Cd & disk);
int main()
{
    Cd c1("Beatles", "Capitol", 14, 35.5);
    Classic c2 = Classic("Piano Sonata in B flat, Fantasia in C",
    "Alfred Brendel", "Philips", 2, 57.17);
    Cd *pcd = &c1;
    cout << "Using object directly:\n";
    c1.Report();
    // use Cd method
    c2.Report();
    // use Classic method
    cout << "Using type cd * pointer to objects:\n";
    pcd->Report(); // use Cd method for cd object
    pcd = &c2;
    pcd->Report(); // use Classic method for classic object
    cout << "Calling a function with a Cd reference argument:\n";
    Bravo(c1);
    Bravo(c2);
    cout << "Testing assignment: " << endl;
    Classic copy;
    copy = c2;
    copy.Report();
    return 0;
}

void Bravo(const Cd & disk)
{
    disk.Report();
}

Code:

Cd& Cd::operator=(const Cd& c)
{
    if(this == &c)
      return *this;
    delete[] performers;
    performers = new char[strlen(c.performers) + 1];
    strcpy(performers, c.performers);
    delete[] label;
    label = new char[strlen(c.label) + 1];
    strcpy(label, c.label);
    selections = c.selections;
    playtime = c.playtime;
    return *this;      
}  

Classic& Classic::operator=(const Classic& cl)
{
   if(this == &cl)
    return *this;
   Cd::operator=(cl);
   delete [] primary_work;
   primary_work = new char[strlen(cl.primary_work) + 1];
   strcpy(primary_work, cl.primary_work);
   return *this;    
}

How are you going to properly copy anything if you delete it first?

1. Allocate new fields
2. Copy into the new fields
3. Delete old fields.

One idiom that is really helpful is to implement the following (I'll do it for your derived object):

Code:

Classical& operator=(Classical const& rhs)
{
   Classical temp(rhs);
   temp.swap(*this);
   return *this;
}

void Classical::swap(Classical const& rhs)
{
    Cd::swap(rhs);
    // char **temp = &rhs.primary_work; -- that doesn't work.
    // rhs.primary_work = primary_work;
    // primary_work = *temp;
    std::swap(primary_work, rhs.primary_work);
}

Classical::Classical(Classical const& rhs): Cd(rhs)
{
    primary_work = new char[strlen(rhs.primary_work) + 1];
    strcpy(primary_work, rhs.primary_work);
}

That looks about right. You have to implement the Cd parts correctly.

How are you going to properly copy anything if you delete it first?

1. Allocate new fields
2. Copy into the new fields
3. Delete old fields.

but whiteflags I've been writing the overloaded operator = that way for another programs and it has been working just fine:

Code:

 
#ifndef DMA_H_ 
#define DMA_H_ 
#include <iostream> 

class BaseDMA
{ 
   private: 
     char* label; 
     int rating;
   public: 
     BaseDMA(const char* l = "null", int r = 0); 
     BaseDMA(const BaseDMA& rs);
     virtual ~BaseDMA();      
     BaseDMA& operator=(const BaseDMA& rs);
     friend std::ostream& operator<<(std::ostream& os, const BaseDMA& rs);
};

// derived class without DMA 
// no destructor needed 
// uses implicit copy constructor 
// uses implicit assignment operator
class LacksDMA : public BaseDMA
{
    private: 
       enum {COL_LEN = 40};
       char color[COL_LEN];
    public: 
       LacksDMA(const char* c = "blank", const char* l = "null", 
                int r = 0);
       LacksDMA(const char* c, const BaseDMA& rs);
       friend std::ostream& operator<<(std::ostream& os, const LacksDMA& rs);
};

// derived class with DMA
class HasDMA : public BaseDMA
{ 
   private:  
     char* style; 
   public: 
      HasDMA(const char* s = "none", const char* l = "null", int r = 0);      
      HasDMA(const char* s, const BaseDMA& rs);
      HasDMA(const HasDMA& hs); 
      virtual ~HasDMA();
      HasDMA& operator=(const HasDMA& hs);
      friend std::ostream& operator<<(std::ostream& os, const HasDMA& hs);
};     
#endif

Code:

 
// implements dma.h
#include "Headers/dma.h"
#include <cstring>

BaseDMA::BaseDMA(const char* l, int r) 
{ 
   label = new char[std::strlen(l) + 1];
   std::strcpy(label, l);
   rating = r; 
}

BaseDMA::BaseDMA(const BaseDMA& rs) 
{ 
   label = new char[std::strlen(rs.label) + 1];
   std::strcpy(label, rs.label);
   rating = rs.rating;    
}     

BaseDMA::~BaseDMA() 
{ 
   delete [] label;    
}     

BaseDMA& BaseDMA::operator=(const BaseDMA & rs)
{
    if(this == &rs)
      return *this;
    delete [] label;
    label = new char[std::strlen(rs.label) + 1];
    std::strcpy(label, rs.label);
    rating = rs.rating;
    return *this;
}

std::ostream & operator<<(std::ostream & os, const BaseDMA & rs)
{
    os << "Label: " << rs.label << std::endl;
    os << "Rating: " << rs.rating << std::endl;
    return os;
}

// LacksDMA methods 
LacksDMA::LacksDMA(const char* c, const char* l, int r) : BaseDMA(l, r)
{
   std::strncpy(color, c, 39);
   color[39] = '\0';     
}

LacksDMA::LacksDMA(const char* c, const BaseDMA& rs) : BaseDMA(rs)
{ 
   std::strncpy(color, c, COL_LEN - 1);
   color[COL_LEN - 1] = '\0';      
}          

std::ostream& operator<<(std::ostream& os, const LacksDMA& ls) 
{ 
   os << (const BaseDMA&) ls; 
   os << "Color: " << ls.color << std::endl; 
   return os;     
}     

// HasDMA methods 
HasDMA::HasDMA(const char* s, const char* l, int r) : 
BaseDMA(l,r)
{ 
    style = new char[std::strlen(s) + 1]; 
    std::strcpy(style, s);
}

HasDMA::HasDMA(const char* s, const BaseDMA& rs) : BaseDMA(rs) 
{ 
  style = new char[std::strlen(s) + 1];
  std::strcpy(style, s);     
} 

HasDMA::HasDMA(const HasDMA& hs) : BaseDMA(hs) 
{ 
   style = new char[std::strlen(hs.style) + 1];
   std::strcpy(style, hs.style);    
}     

HasDMA::~HasDMA() 
{ 
   delete [] style;     
}             

HasDMA& HasDMA::operator=(const HasDMA& hs) 
{ 
   if(this == &hs) 
     return *this; 
   BaseDMA::operator=(hs);
   delete [] style; 
   style = new char[std::strlen(hs.style) + 1];
   std::strcpy(style, hs.style);
   return *this;       
}     

std::ostream & operator<<(std::ostream & os, const HasDMA & hs)
{
    os << (const BaseDMA &) hs;
    os << "Style: " << hs.style << std::endl;
    return os;
}

Code:

 
// main program -- compile with dma.cpp
#include <iostream> 
#include "Headers/dma.h"

int main()
{
    using std::cout;
    using std::endl;
    
    BaseDMA shirt("Portabelly", 8);
    LacksDMA balloon("red", "Blimpo", 4);
    HasDMA map("Mercator", "Buffalo Keys", 5);
    cout << "Displaying baseDMA object:\n";
    cout << shirt << endl;
    cout << "Displaying lacksDMA object:\n";
    cout << balloon << endl;
    cout << "Displaying hasDMA object:\n";
    cout << map << endl;
    LacksDMA balloon2(balloon);
    cout << "Result of lacksDMA copy:\n";
    cout << balloon2 << endl;
    HasDMA map2;
    map2 = map;
    cout << "Result of hasDMA assignment:\n";
    cout << map2 << endl; 
    
    return 0; 
}

Code:

 
thames@semaht ~/C++/Projects/DynamicMemory $ g++ -g -Wall dma.cpp usedma.cpp -o dma -std=c++11
thames@semaht ~/C++/Projects/DynamicMemory $ valgrind ./dma
==2887== Memcheck, a memory error detector
==2887== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
==2887== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info
==2887== Command: ./dma
==2887== 
Displaying baseDMA object:
Label: Portabelly
Rating: 8

Displaying lacksDMA object:
Label: Blimpo
Rating: 4
Color: red

Displaying hasDMA object:
Label: Buffalo Keys
Rating: 5
Style: Mercator

Result of lacksDMA copy:
Label: Blimpo
Rating: 4
Color: red

Result of hasDMA assignment:
Label: Buffalo Keys
Rating: 5
Style: Mercator

==2887== 
==2887== HEAP SUMMARY:
==2887==     in use at exit: 0 bytes in 0 blocks
==2887==   total heap usage: 9 allocs, 9 frees, 79 bytes allocated
==2887== 
==2887== All heap blocks were freed -- no leaks are possible
==2887== 
==2887== For counts of detected and suppressed errors, rerun with: -v
==2887== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 2)

Stephen Prata coded the operator function that way to get rid of the old data for new memory allocation. After all, I'm renewing the memory space, right?

Originally Posted by thames

Stephen Prata coded the operator function that way to get rid of the old data for new memory allocation. After all, I'm renewing the memory space, right?

Well I've gotten away from doing it that way for tons of reasons. One, I constantly do it wrong, and two, whenever I show that kind of code I always get complaints about exception safety. If new actually fails and throws an exception, then you've already changed the object that you were assigning to, by deleting some of the fields.

Exceptions usually come with guarantees. I forget the names (yes, they have names), but it works as follows:
1. Exceptions will not be thrown.
2. Exceptions may be thrown, but the objects are guaranteed to be unchanged in that event.
3. No exception guarantees.

Level 1 is good because there is no stack tracing ever. Level 2 is good (and practical) because if you do a stack trace, you can actually figure out where the error occurred. Level 3 is complete chaos. The way Stephen Prata showed you is 3.

I would change your operator just to see if it helps remove the problems from valgrind.

But now that I look at it, can you explain this?

Code:

    Cd c1("Beatles", "Capitol", 14, 35.5);
    Classic c2 = Classic("Piano Sonata in B flat, Fantasia in C",
    "Alfred Brendel", "Philips", 2, 57.17);

c1 is OK. But for c2, you allocated a default object, then assigned a temp object to it. Was there a reason for that? In any case, the copy assignment operator that I showed you works properly for that use as well.

whiteflags can you tell me if I need to define code for those default constructors ?

Code:

Classic() {} 
Cd() {}

edit:

c1 is OK. But for c2, you allocated a default object, then assigned a temp object to it. Was there a reason for that?

err ... that main is part of an exercise from Stephen Prata's book. I must not change main. Also, I did this program earlier with static allocation with that main.

Yes you do. A constructor that does nothing is not good enough.

Originally Posted by whiteflags

Yes you do. A constructor that does nothing is not good enough.

how should I do this when it comes to that case, dynamic memory allocation? I used gdb and I think there's garbage inside performers and label when the object is created:

Code:

Classic copy;

Code:

 
(gdb) print copy.performers
$5 = 0x40189d <__libc_csu_init+93> "H\203\303\001H9\353u\352H\213\\$\bH\213l$\020L\213d$\030L\213l$ L\213t$(L\213|$0H\203\304\070\303\017\037\200"
(gdb) print copy.label
$6 = 0x7ffff7ae1998 "@\021"

before, I wrote some code for a constructor which had defined static allocated arrays.

Well the easy thing would be to not do it, but since you can't change main, that's impossible. You will have to assign memory to all the pointers and come up with values for the fields in both classes in the default constructors.

I fixed white. Many thanks!

Code:

 
Cd::Cd()
{ 
   performers = new char[1]; 
   performers[0] = '\0'; 
   label = new char[1]; 
   label[0] = '\0';
   selections = 0; 
   playtime = 0.0;     
}

Code:

 
Classic::Classic()
{ 
   primary_work = new char[1]; 
   primary_work[0] = '\0';     
}

Hmm, my preferences would have been "(null)" or "<?>" and 0, but whatever works, I guess.

Originally Posted by whiteflags

Hmm, my preferences would have been "(null)" or "<?>" and 0, but whatever works, I guess.

wha? can you show me a code snippet?

Psst: Allow me to save you a lot of time and headaches:
Learn about the "Copy and Swap Idiom".

GotW #59: Exception-Safe Class Design, Part 1: Copy Assignment

Stephen Prata is known for not writing good C++ books.
If you are reading one, I strongly suggest you switch. I recommend this book instead.
This code is just unacceptable. You've just been thrown into the C world within C++, having to deal with memory management, copying and exception safety issues at extreme level. You shouldn't have to! This source you are learning from is complete and utter crap. You deserve better.
Instead of using char* for string, use std::string. Now you don't need new, delete, assignment operators, copy constructors, destructors and all that stuff. I suggest you re-write it using std::string unless it is a strict requirement that you must not.

I read this review before.

I'm almost at the end of Stephen Prata's book (page 830). At least, I've been learning some principles of C++. He has been using char* and char[] a lot, I admit. Based on what you told me and the review I found, I couldn't decide what to do until the programs using std::string started to appear. (there are some of then too). My guess is that he shows both sides of the coin so the C++ principles get stuck in your head (I think).