Would this constructor be acceptable practice?

Question

Let's assume I have a c++ class that have properly implemented a copy constructor and an overloaded = operator. By properly implemented I mean they are working and perform a deep copy:

Class1::Class1(const Class1 &class1)
{
  // Perform copy
}
Class1& Class1::operator=(const Class1 *class1)
{
  // perform copy
  return *this;
}

Now lets say I have this constructor as well:

Class1::Class1(Class1 *class1)
{
   *this = *class1;
}

My question is would the above constructor be acceptable practice? This is code that i've inherited and maintaining.

Answer 1

Personally, I don't think it's good practice.

For the constructor, it's hard to think of a place where an implicit conversion from a pointer to an object to the object itself would be useful.

There's no reason for the pointer to be to non-const, and if you have available pointer to the class it is not hard to dereference it, and so clearly state your intention of wanting to copy the object using the copy constructor.

Similarly, for the non-standard assignment operator why allow assignment from a pointer when correctly dereferencing at the call site is clearer and more idiomatic?

Answer 2

I would say, "no", for the following reasons:

• A traditional copy constructor accepts its argument as a const reference, not as a pointer.
• Even if you were to accept a pointer as a parameter, it really ought to be const Class1* to signify that the argument will not be modified.
• This copy constructor is inefficient (or won't work!) because all members of Class1 are default-initialized, and then copied using operator=
• operator= has the same problem; it should accept a reference, not a pointer.

The traditional way to "re-use" the copy constructor in operator= is the copy-and-swap idiom. I would suggest implementing the class that way.

Answer 3

Objects and pointers to objects are two very different things. Typically, when you're passing objects around, you expect that they're going to be copied (though, ideally functions would take const refs where possible to reduce/eliminate unnecessary copies). When you're passing a pointer around, you don't expect any copying to take place. You're passing around a pointer to a specific object and, depending on the code, it could really matter that you deal with that specific object and not a copy of it.

Assignment operators and constructors that take pointers to the type - especially constructors which can be used for implicit conversion - are really going to muddle things and stand a high chance of creating unintended copies, which not only could be a performance issue, but it could cause bugs.

I can't think of any good reason why you would ever want to make the conversion between a pointer to a type and the type itself implicit - or even explicit. The built-in way to do that is to dereference the object. I suppose that there might be some set of specific circumstances which I can't think of where this sort of thing might be necessary or a good idea, but I really doubt it. Certainly, I would strongly advise against doing it unless you have a specific and good reason for doing so.

Answer 4

I believe a somewhat more important issue than what has been discussed so far is that your non-standard assignment operator does not stop the compiler from generating the standard one. Since you've decided that you need to create an assignment operator (good bet since you made the copy constructor), the default is almost certainly not sufficient. Thus a user of this class could fall prey to this problem during what would seem very basic and standard use of an object to almost anyone.