Should I check for NULL in my assignment operator?

Question

I have a class

class Node {

  public:
    int value;
    Node * next;

    Node();
    Node(const Node& other);
    Node& operator= (const Node& other);
};

It's not very useful, but it has an overridden assignment operator. Everything in there is public because I am such an open and cooperative guy.

Now elsewhere I have an array of these nodes:

Node * nodes = new Node[15];

When I try to assign a node to my array of nodes:

nodes[0] = Node();

I get a huge ugly crash.

My assignment operator looks like this:

Node& Node::operator= (const Node& other) {

  // watch out for self assignment
  if (this == &other) return *this;

  delete this->next;
  this->next = new Node(*(other.next)); // call the copy constructor
  this->value = other.value;

  return *this;
}

I get the feeling that I should be checking for whether or not this is NULL before I go about trying to dereference its members. Any thoughts on what might be wrong?

`*(other.node)` should not compile. `Node` doesn't have a member `node`. — CB Bailey, Oct 07 '11 at 23:23
Post edit: `delete this->node` also should not compile. `Node` doesn't have a member `node`. Perhaps you haven't posted the real code? — CB Bailey, Oct 07 '11 at 23:27
Aha ha, yes. Thanks. No this isn't the real code, this is just enough to express the situation. — Ziggy, Oct 07 '11 at 23:29
Also, your "delete + assign new" pattern is not exception safe. — asveikau, Oct 07 '11 at 23:43
@asveikau Why is "delete + assign new" not exception safe? It sounds interesting, and I'd love to know! — user, Oct 08 '11 at 01:23
@Oliver: Generally, you want an operation to do everything, or do nothing. But what happens if the `new Node` line throws an exception? `next` has already been deleted, but a new value has not been assigned. You're left half done. — Dennis Zickefoose, Oct 08 '11 at 02:06
@Oliver - @DennisZickefoose has already done a good job of describing why. If the `new` throws, the `delete` has already happened, and you're left with a stale pointer. — asveikau, Oct 08 '11 at 09:08

score 2 · Answer 1 · answered Oct 07 '11 at 23:37

You should never check that this is NULL; it is illegal to call a non-static member function other than on a valid object.

You may have to ensure that the next pointer member variables of both source and destination objects in the assignment are either null or point to valid objects. Without seeing the real code it is impossible to say whether the constructors that you have at the moment do this correctly.

Chris Dodd · Accepted Answer · 2011-10-07T23:44:42.950

1

The problem is that you're dereferencing other.next, which might be NULL. So you should check if other.next is null before dereferencing it:

this->next = other.next ? new Node(*other.next) : 0;

Dereferencing a NULL pointer is undefined, so if you do it, anything might happen -- it might NOT crash immediately and instead wander off into unexpected places, confusing both you and the debugger.

edited Oct 07 '11 at 23:44

answered Oct 07 '11 at 23:38

Chris Dodd

119,907
13
134
226

Dereferencing a null pointer is undefined, but will certainly always trigger a segmentation fault. Dereferencing an _uninitialized_ pointer on the other hand may have strange results. – Oscar Korz Oct 07 '11 at 23:47
1

@okorz001: Both operations cause _undefined behavior_, I don't see any reason to expect anything other than strange results from either. – CB Bailey Oct 07 '11 at 23:59
@okorz001 Depends on your architecture and userland/kernelspace, but you can deference a NULL pointer. – ta.speot.is Oct 08 '11 at 00:00
Even better than my answer. Straight to the problem. – sanosdole Oct 08 '11 at 22:26

sanosdole · Answer 3 · 2011-10-08T01:20:24.600

0

I double that you should not check this for NULL. I believe your ownership is not clear set up here. Who owns the next node?

This ->next or other ->next might be null, so check before delete/dereference.

Without knowing more about your code it is difficult to say what you are trying to achieve.

edited Oct 08 '11 at 01:20

answered Oct 08 '11 at 00:06

sanosdole

2,469
16
18

score -1 · Answer 4 · answered Oct 07 '11 at 23:58

-1

this is never going to be NULL, however in your example this->next is going to be either NULL or an invalid reference for nodes[0] as this object has never been initialized (I'm not quite sure what the compiler does, perhaps it fills the array with zeros, perhaps not). I suggest you guard against delete this->next; for NULL pointers, and that you make sure your array is blanked after you allocate it. I.e. make these changes:

Node * nodes = new Node[15] {0};

And

if (this->next) delete this->next;

answered Oct 07 '11 at 23:58

Clafou

15,250
7
58
89

2

-1: IMO this is bad advice -- the right way to ensure that `next` is not uninitialized is to initialize it in all Node constructors. Checking a pointer to see if its null before calling delete is similarly pointless -- delete of a null pointer is a well-defined noop – Chris Dodd Oct 08 '11 at 00:06
Fair point on the delete of null being safe (although saying that delete of a null pointer is a noop and guaranteed to be safe is an assumption that may not be always correct, perhaps some implementations are intentionally dumb). However in this case the Node object at nodes[0] is not initialized by any constructor as it comes from an uninitialized array allocation. I assume this array contains indeterminate data, causing delete this->next to crash. – Clafou Oct 08 '11 at 00:31
The default ctor of node is called from newing up the array. And a initializer for next is a must. Uninitialized pointers are a certain way to hell! – sanosdole Oct 08 '11 at 01:17
We haven't seen the default ctor but it does appear that it omits to initialize the next pointer. I can see two options: changing the default ctor to initialize next, or zero-initializing the array as in my answer. I don't claim the latter is a better option but it's one way to fix the crash. So rather than downvoting me upvoting the other answer would seem fair. – Clafou Oct 08 '11 at 12:37
Zero initializing won't work, as there's no Node ctor that takes an int. Even if you add such a Node(int) ctor, you still need to initialize the next pointer in that ctor, or it won't be initialized. As it is though, trying to use your suggestion will just cause a compile time error. – Chris Dodd Oct 10 '11 at 02:10
Apologies for the compile error. What I meant is 'Node nodes[15] = {};' which is the correct syntax to declare and zero-initialize. – Clafou Oct 10 '11 at 18:35

Should I check for NULL in my assignment operator?

4 Answers4