1

I have a class A which dynamically allocates memory for an integer(pointed by a class memeber say _pPtrMem) in its constructor and deallocates the same in destructor. To avoid Shallow copy, I have overloaded assignment operator and copy constructor. The widely used way in which assignment operator is overloaded is as following:

A& operator = (const A & iToAssign)
{
  if (this == & iToAssign)  // Check for self assignment
    return *this;
  int * pTemp = new int(*(iToAssign._pPtrMem)); // Allocate new memory with same value
  if (pTemp)
  {
    delete _pPtrMem;  // Delete the old memory
    _pPtrMem = pTemp; // Assign the newly allocated memory
  }
  return *this;   // Return the reference to object for chaining(a = b = c)
}

Another way for implementing the same could be

A& operator = (const A & iToAssign)
{
  *_pPtrMem= *(iToAssign._pPtrMem); // Just copy the values
  return *this;   
}

Since the second version is comparatively much simpler and faster(no deallocation, allocation of memory) why is it not used widely? Any problems that I am not able to make out?

Also we return an object of the same type from the assignment operator for chaining(a = b = c)... so instead of returning *this is it fine to return the iToAssign object as both objects are supposedly now equal?

Arun
  • 3,138
  • 4
  • 30
  • 41
  • 2
    If you just have a single integer, why bother using pointers? If you have dynamically allocated arrays, consider using `std::vector` instead. – Some programmer dude Mar 12 '13 at 07:57
  • 1
    Note: in version 1 the `delete` is too early. It should be done after the current object is in stable state (otherwise you do not provide the strong exception guarantee). Here it is not stable because you assign to the current object after the delete. You should `std::swap(_pPtrMem, pTemp);delete pTemp;`. Better yet use the copy and swap idiom. – Martin York Mar 12 '13 at 08:32
  • @LokiAstari but assigning pointers can't throw an exception, there is nothing that can go wrong. – Torsten Robitzki Mar 12 '13 at 08:50
  • @TorstenRobitzki: But the delete can throw (if the object being deleted has a destructor). So to be consistent always put the delete after you have finished changing the object (that way if you later modify the contained type you don't need to change the code in the assignment). But you are correct for POD types like `int` which is why this is just a comment. – Martin York Mar 12 '13 at 09:03
  • @LokiAstari The `delete` is _not_ too early (but the test for self assignment is not necessary). If the `delete` can throw, there is _no_ way you can implement this safely; that's why destructors should never throw. (There are a few special exceptions, but classes whose destructor might throw should never be used as part of another class or put into a container.) – James Kanze Mar 12 '13 at 09:39
  • @JamesKanze: No argument that destructors should not throw. But they can and do. **BUT** you can still implement this with the "Strong Exception Guarantee" by making sure the delete is done after the object has completed changing its state. If you do the delete as shown above you are not providing guarantees. If you do it after (as shown in my comment) then you have provided the "Strong Guarantee". The state of data not part of the object (in terms of the object) is irrelevant as long as the object is consistent. – Martin York Mar 12 '13 at 18:17
  • @LokiAstari In this case, with just a single pointer, you can easily implement the strong guarantee, with or without swap. If you have more than one pointer, however, and the destructor of both objects may throw, there is no way you can implement even the weakest guarantee. The code cannot be made to work. – James Kanze Mar 12 '13 at 18:45
  • @JamesKanze: OK I'll bite. how do you implement the strong guarantee here. If you don't move the delete. – Martin York Mar 12 '13 at 19:33
  • @LokiAstari First, as written, it already implements the strong guarantee. If you insist on supporting a destructor which throws (which you can't in general), just swapping the pointers before the delete is sufficient. There's no need to go through the rigamarole of defining a new function. – James Kanze Mar 13 '13 at 08:59
  • @JamesKanze: As written it definitely does not implement the strong guarantee (for non POD types) as it may throw and leave the object in an undefined state (as the object will still be pointing at memory that has been "deleted" when the exception starts propagating). I am not sure where you see an extra function (you seem to be meandering off the point). If you read my comment above I said `std::swap(_pPtrMem, pTemp);delete pTemp;`. – Martin York Mar 13 '13 at 15:23
  • @LokiAstari The normal practice is to ban exceptions from destructors, because without that, you cannot, in general, implement even a weak guarantee. The first version in the OP implements the strong guarantee according to the usual rules. – James Kanze Mar 14 '13 at 08:34
  • @LokiAstari For the rest, I was (perhaps mistakenly) under the impression that you were arguing for the classical swap idiom, which requires an extra function (member `swap`). If this is not the case, then I think we agree. In this particular case (but not in general), you can support the destructor throwing, by swapping the pointers before the delete. Still, I would consider such unnecessary added complexity, because the rule is that you can assume destructors don't throw. – James Kanze Mar 14 '13 at 08:35
  • And for what it's worth, the original code also has some unnecessary additional complexity: there's no need for a test for self assignment, and since `new` can never return a null pointer, there's no need to test that either. – James Kanze Mar 14 '13 at 08:36

5 Answers5

6

Usually, the best way to implement a copy assignment operator is to provide a swap() function for your class (or use the standard one if it does what you want) and then implement the copy assignment operator via the copy constructor:

A& A::operator= (A iToAssign)  // note pass by value here - will invoke copy constructor
{
  iToAssign.swap(*this);
  return *this;
}
// void swap(A& other) throws() // C++03
void A::swap(A& other) noexcept
{
    std::swap(_pPtrMem, other._pPtrMem);
}

This makes sure that your copy assignment operator and copy constructor never diverge (that is, it cannot happen that you change one and forget to change the other).

Angew is no longer proud of SO
  • 167,307
  • 17
  • 350
  • 455
  • 2
    *"or use the standard one if it does what you want"* -- It seems to me that would lead to infinite recursion, since `std::swap` relies on `operator=`. – Benjamin Lindley Mar 12 '13 at 08:21
  • @LokiAstari It's not the same. If you provide your own overload of `swap()` for your class, the `using std::swap` idiom allows this overload to be found via ADL, and only if no such thing exists does the "`using`-ed" `std::swap` kick in. – Angew is no longer proud of SO Mar 12 '13 at 08:27
  • @Angew: Yes, figured it out. But you can't use std::swap in the assignment operator (as per Benjamins comment). So you must define your own (or do the swap manually in the assignment). So there is again no point in putting the using statement in. – Martin York Mar 12 '13 at 08:29
  • IMHO it is more obvious to pass by const ref and then invoke the copy constructor inside your function; I know it's exactly the same semantically, but it would have prevented me thinking "Oi, you're modifying the RHS!" – Alex Chamberlain Mar 12 '13 at 08:31
  • 2
    @AlexChamberlain: But then you inhibit both move semantics and copy elision in the case where the RHS is an R-value. – Benjamin Lindley Mar 12 '13 at 08:32
  • @BenjaminLindley Would you? Not sure I understand; anywhere I can learn more? – Alex Chamberlain Mar 12 '13 at 08:35
  • @AlexChamberlain: I (initially) agree. The first time I saw the pass by value I had to do a double take (and work out what was happening). But this form has become rather standard because of the optimizing benefits. So now I write it like this but always put a comment about pass by value. – Martin York Mar 12 '13 at 08:36
  • 1
    @AlexChamberlain: Also, [this fairly famous article.](http://cpp-next.com/archive/2009/08/want-speed-pass-by-value/) – Benjamin Lindley Mar 12 '13 at 08:39
  • It's debatable whether copy-and-swap is *the best* way to implement the assignment operator. It certainly is very competent, and strikes a nice balance between performance and ease of implementation/maintenance. However, the fact that it does all the same work for the (admittedly rare) case of self assignment, instead of just returning like traditional implementations, keeps it from being declared indisputably best. – Benjamin Lindley Mar 12 '13 at 08:52
  • @BenjaminLindley I've weakened the claim a bit. – Angew is no longer proud of SO Mar 12 '13 at 08:54
  • @AlexChamberlain, the tso are not semantically identical (const ref argument and copy&swap). Besides the copy&swap supporting copy elision and move semantics, it is also a lot more natural to use copy&swap to provide (weak or strong) exception guarantees in the assignment operator implementation. – utnapistim Mar 12 '13 at 09:45
  • Why is using swap the best way? His first version is perfectly fine, and accomplishes everything the swap version does. – James Kanze Mar 12 '13 at 09:46
  • @JamesKanze Exception safety is much easier to achieve with the copy&swap. So is constructor-operator consistency. – Angew is no longer proud of SO Mar 12 '13 at 10:06
  • @utnapistim I was still proposing copy & swap, but move the copy inside the function for clarity. – Alex Chamberlain Mar 12 '13 at 10:21
  • I don't think it would work (if I were to see a function that takes a const& and makes a local copy it would confuse me -- why didn't you take the parameter by value instead?). I think it would also disable potential compiler optimizations. – utnapistim Mar 12 '13 at 10:44
  • @utnapistim It would disable optimisation (according to comments above), but it would also certainly work. – Alex Chamberlain Mar 12 '13 at 10:45
  • :) I meant it wouldn't work to make the code clearer - it would confuse me instead. – utnapistim Mar 12 '13 at 10:47
  • @Angew How is exception safety any easier than in his first example? (The copy constructor/copy assignment operator consistency is an argument, but it doesn't really apply until you have a number of members which have to be handled consistently.) – James Kanze Mar 12 '13 at 11:54
  • @utnapistim The general coding guidelines are to pass class types by const reference. When one sees something different in the interface, one asks why. – James Kanze Mar 12 '13 at 11:55
  • @JamesKanze, it is also a guideline to pass by value if you always need a copy of the data. That is, if in all cases, your implementation needs to instantiate a copy of the input data, you should pass by value (and have the compiler instantiate the copy), instead of passing by const reference and creating the copy yourself. (edit: I will try looking for some authoritative reference for this; if I find any, I will post it here). – utnapistim Mar 12 '13 at 12:03
  • @utnapistim It sounds like it would be a reasonable guideline, although I've never seen it. On the other hand, it does mean that the actual form of the publicly visible interface depends on the internal implementation. I can imagine some people not liking this. (Change the implementation, and all of your users get to recompile.) – James Kanze Mar 12 '13 at 12:36
  • "Change the implementation, and all of your users get to recompile." - true (and this is a case when you probably shouldn't break the interface for an optimization). I was thinking of less gray areas though (say a case like `std::string shuffled_copy(std::string seed);`; In such a case (when you are clearly and intentionally making a copy), you might as well make it in the argument list. – utnapistim Mar 12 '13 at 12:57
2

No, there is no problem with your implementation. But having one integer dynamic allocated is at least very special.

This implementation is not widely used, because no one allocates a single integer on the free store. You usually use dynamic allocated memory for arrays with a variable length unknown at compile time. And in this case it's most of the time a good idea to just use std::vector.

No it's not fine, to return an different object. Identity is not the same as equality:

T a, b, d;
T& c = a = b;
c = d; // should change a, not b

Would you expect that the third line changes b?

Or a event better Example:

T a;
T& b = a = T();

This would result in a dangling reference, referencing an temporary and destructed object.

Torsten Robitzki
  • 3,041
  • 1
  • 21
  • 35
1

The first version is used in case _pPtrMem is a pointer to some basic type for instance a dynamically allocated array. In case the pointer is pointing to a single object with correctly implemented assignment operator the second version will do just as good. But in that case I don't think you will need to use a pointer at all.

Ivaylo Strandjev
  • 69,226
  • 18
  • 123
  • 176
0

In the second case, if _pPtrMem was initially unassigned, the line

*_pPtrMem= *(iToAssign._pPtrMem); // Just copy the values

is causing an assignment to an invalid memory location (possibly a segmentation fault). This can only work if _pPtrMem has been allocated memory prior to this call.

uba
  • 2,012
  • 18
  • 21
  • 1
    *"I have a class A which dynamically allocates [...]`pPtrMem` [..] in its constructor"*. – Zeta Mar 12 '13 at 08:00
0

In this case, the second implementation is by far the better. But the usual reason for using dynamic in an object is because the size may vary. (Another reason is because you want the reference semantics of shallow copy.) In such cases, the simplest solution is to use your first assignment (without the test for self-assignment). Depending on the object, one might consider reusing the memory already present if the new value will fit; it adds to the complexity somewhat (since you have to test whether it will fit, and still do the allocation/copy/delete if it doesn't), but it can improve performance in certain cases.

James Kanze
  • 150,581
  • 18
  • 184
  • 329