C++ Compiler Error: No matching function for call

Question

Look at the following code. What is wrong with it? The compiler gives the this error:

In copy constructor person::person(person&)': No matching function for call toperson::copy(char*&, char*&)' candidates are: void person::copy(char*&, const char*&) "

Here is the code:

class person
{
  public:
    person();
    person(person &);

  private:
    void copy(char*&,const char*&);
    char* name, *fathername,* address;
};

void person::copy( char*& n, const char*& p)
{
  int result;
  result=strcmp(n,p);
  if(result!=0)
  {
    n=new char[strlen(p)+1];
    strcpy(n,p);
    n[strlen(p)]='\0';
  }
}
person::person(person &object)
{
  copy(name,object.name);
  copy(fathername,object.fathername);
  copy(address, object.address);
}

From the answers to this Question what I understood up until now is given by: the compiler does not allow to convert a reference to a constant reference because references are already constant. They can't point to a different memory location like pointer. Am I right?

Answer 1

Change

person::person(person &object)

to

person::person(const person &object)

for starters...

Answer 2

The compiler is telling you the problem - change your signature to accept 2 char* pointers (rather than 1 const char*) and it should compile.

The issue is really due ot the use of the reference - if you had created a copy method that simply took 2 char* pointers (not references) then the compiler will automatically recognise the conversion from char* to const char* and use the method. As you only have a method that accepts a reference to a different type it cannot do that automatically.

Answer 3

Wouldn't this be nicer?

class person
{
private:
  std::string name;
  std::string fathername
  std::string address;
};

// constructor and copy constructor autogenerated!

It's more "C++" this way ;).

Answer 4

Unless you are planning on changing the pointers you should not pass the references to the pointers:

Change:

void person::copy( char*& n, const char*& p) 

to

void person::copy( char* n, const char* p) 

This is because p is a reference to a particular type.
The object you passed is not the exact type and becuase it is a reference their is no way to convert it.

The change I suggested above allows for a "pointer to const char" (p) thus allowing read only access to elements via 'p'. Now a "pointer to char" allows read/write access to the data so converting this to "pointer to const char" is allowed because we are just limiting the allowed behavior.

There are a whole set of other problems with the code you posted.
Do you want us to list them?

I don't do NOW. I do on my schedule.

Problems:

1: You leak on each call to copy:

if(result!=0)
{
    n=new char[strlen(p)+1];   // What happned to the old n?

2: The default assignment operator is used.

person a;
person b;
a = b; // a.name == b.name etc all point at the same memory location.
       // Though because you do not delete anything in the destructor
       // it is technically not an issue yet.

3: You done delete the allocated members in the destructor.

{
     person  a;
} // A destructor called. You leak all the member here.

4: strcpy() already copies the terminating '\0' character.

5: if the call to new throws an exception. You will leak memory.

copy(name,object.name); 
copy(fathername,object.fathername);   // If new throws in here.
                                      // Then the this.name will be leaked.

Doing this correctly using C-String is so hard that even a C++ expert would have problems doing this correctly. Thats why C++ experts would use std::string rather than a C-String. If you must use C-Strings then you should wrap your C-String in another class to protect it from probelms with exceptions.

Answer 5

I'm feeling generous, so here is a corrected version of your code:

class person
{
public:
    person();
    person(const person &);
    ~person();
private:
    void copy(char*&,   // Do you understand purpose of '&' here?
              const char*);
    char* name;
    char* fathername;
    char* address;
};

person::person()
    : name(NULL),
      fathername(NULL),
      address(NULL)
{
}

person::~person()
{
    delete[] name;
    delete[] fathername;
    delete[] address;
}

void person::copy( char*& n,  // The '&' is required because the contents of `n` are changed.
                   const char* p)
{
    delete[] n;
    n = NULL;     // Here is one place where contents of `n` are changed.
    if (p)
    {
        n = new char [strlen(p) + sizeof('\0')];  // Another content changing location.
        strcpy(n, p);
        n[strlen(p)]='\0';
    }
}

person::person(const person& object)
{
    copy(name,object.name);
    copy(fathername,object.fathername);
    copy(address, object.address);
}

Can you identify the flaws or safety items still lurking?

Answer 6

As others are saying, you shouldn't pass the char pointer by reference if you are not going to modify it.

The problem is that the reference is non-const and therefore doesn't bind to temporaries. Therefore the passed variable's type must match exactly. Near matches that would involve an implicit cast are not acceptable, because the result of an implicit cast is a temporary.

Const references, on the other hand, can be bound to temporaries.

void non_constant(int&);
void constant(const int&);

int main()
{
    int i = 0;
    unsigned u = 0;
    non_constant(i);
    //non_constant(u);  //ERROR: not an int
    //non_constant(10);  //ERROR: literals are temporaries
    constant(i);
    constant(u);  //OK, unsigned implicitly cast to int
    constant(10); //OK, literals bind to const references
}

So, if you wanted badly to keep the reference in the argument:

void person::copy( char*& n, const char* const& p)

Answer 7

This is VERY (!) poor design. This code is buggy and VERY (!) hard to understand and maintain. This question is continuation for this question: http://stackoverflow.com/questions/2108389/c-classes-object-oriented-programming/2108601#2108601.

Now you struggling with symptoms, not with real problem. And real problem is to think in C++ terms not in C (if you want to became C++ object-oriented programmer).

Valid C++ code (C++, not C with classes) here:

#include <string>

class person
{
public:
  person();
private:
  std::string name, fathername, address;
};

Thats all. All other things (including copy contstructor) C++ compiler generates for you (as well effective as you own manual implementation)! This much simpler, much clearer, easier to maintain and understand, and first of all: bug free;). And this is true C++ code.

Answer 8

Others pointed that you should replace reference with pointer.

Here are a few other but related comments:

1. If you define copy constructor, define assignment operator too. They should go along together, in most cases.

2. It's a good practice to declare single argument constructor as explicit.

3. Naming objects as object is a bad convention and may lead to confusions. It's abvious every instance of type person is an object. Use more meaningful names, like person(const person& other); or person(const person& rhs); // after right-hand-side

4. Use std::string. If you are programming in C++, there is no rational reason to not to use std::string and juggle C-strings instead.

5. Finally, take care of exception safety, follow best practices like copying oprations implemented in terms of non-throwing swap operation. See article Exception-Safe Class Design, Part 1: Copy Assignment by Herb Sutter