TempNewSymbol analysis


1)

TempNewSymbol first = SymbolTable::new_symbol(mystring, CHECK);

Result:

 1 TempNewSymbol and 1 Symbol with _refcount == 1

Comment:

SymbolTable::new_symbol(module_location, CHECK); evaluates to a Symbol* with a refcount of 1
Since TempNewSymbol contains a constructor taking a Symbol*:

TempNewSymbol(Symbol*s) : _temp(s) {}

The compiler will select this constructor from the context provided by the Symbol*. This means 
there is a type conversion in effect, mapping an existing Symbol* to new TempNewSymbol.
Since the TempNewSymbol(Symbol*s) does not increment _refcount, all is good.

2)

TempNewSymbol mysymbol;
...
mysymbol = SymbolTable::new_symbol(mystring, CHECK);

Result:
Self-assignment:
 1 TempNewSymbol and 1 Symbol* with _refcount == 2

For non-self assignment:
 1 TempNewSymbol and 1 Symbol* with _refcount == 1   

Comment:

Here is an assignment from a Symbol* to an existing TempNewSymbol instance. The compiler 
checks for an appropriate assignment operator for the context provided by the Symbol*, and finds:

void operator=(const TempNewSymbol& s)

As described in 1, there exists a conversion operator/constructor from a Symbol* to a 
TempNewSymbol. Therefore, it will be able to use the above assignment

 void operator=(const TempNewSymbol& s)

In order to do so, the compiler must first create a new temporary  TempNewSymbol instance:

TempNewSymbol temporary(Symbol*);

// as explained in 1, this constructor does not increment _refcount - at this point there is only one 
(temporary) TempNewSymbol and one Symbol*.

// Operator= increments reference count.
void operator=(const TempNewSymbol &s) {
  //clear();  //FIXME
  _temp = s._temp;
  if (_temp !=NULL) _temp->increment_refcount();
}

Will talk more about the missing clear() in 4. For now, let's just see what is going on:
Symbol* is assigned to mysymbol and the Symbol* reference count is updated, i.e. now _refcount == 2.
After operator=(const TempNewSymbol &s) has completed, the compiler generated temporary 
(for type conversion) is destroyed, bringing back the _refcout to 1, which is correct provided that
there is no self-assignment here, i.e. this != &s:

In case of self-assignment, i.e. this == &s, now we have a resource leak, since we have updated "us" twice.

3)

TempNewSymbol second = first;

Result:
Current:

 2 TempNewSymbol's and 1 Symbol* with _refcount == 1

Fixed:
 2 TempNewSymbol's and 1 Symbol* with _refcount == 2

Comment:

This expression will call the copy constructor for TempNewSymbol. Since there is currently none 
explicitly provided, it will call the compiler generated default copy constructor. Since this means 
shallow copy of every member, we are not incrementing the _refcount of the Symbol* correctly. This sets 
up a situation for reference counting underflow:

 2 TempNewSymbol and 1 Symbol* with _refcount == 1;

Since refcounts are dropped in TempNewSymbol's destructor, this means _refcount will be 
decremented twice, since the second destructor to run will bring refcount to -1 (fortunately we 
have an existing Symbol* _refcount underflow assertion in place to guard for this).

An explicitly overloaded copy constructor that correctly increment the Symbol* _refcount will be 
the right thing to do here:

// Copy constructor increments reference count.
TempNewSymbol(const TempNewSymbol& rhs) {
  _temp = rhs._temp;
  if (_temp != NULL) {
    _temp->increment_refcount();
  }
}

4)

TempNewSymbol first = SymbolTable::new_symbol(mystring, CHECK);
TempNewSymbol second = SymbolTable::new_symbol(mystring, CHECK);
first = second;

Result:

Current:
Non-self assignment:
 1 Symbol* not decremented, i.e. resource leak on first Symbol*
 2 TempNewSymbol's and 1 Symbol* with _refcount == 2

Fixed:
 1 Symbol* now decremented, i.e. no resource leak on first Symbol*
 2 TempNewSymbol's and 1 Symbol* with _refcount == 2

Comment:

Without some resource management routine like the missing clear(), this is obviously a resource leak:

// Operator= increments reference count.
void operator=(const TempNewSymbol &s) {
  //clear();  //FIXME
  _temp = s._temp;
  if (_temp !=NULL) _temp->increment_refcount();
}

The existing Symbol* is being overwritten without the necessary decrement of the Symbol* refcount.
Adding and calling a "clear()" member function will provide for the correct decrement before overwrite:

// Decrement reference counter so it can go away if it's unique
void clear() {
  if (_temp != NULL) {
    _temp->decrement_refcount();
    _temp = NULL;
  }
}

Only concern is why //clear();  //FIXME was not implemented in the first place? Maybe this was 
done because it is unclear what side-effects this fix might be causing if existing logic depend on this 
leaking behavior? However, the right thing to do is to find those sites and correct them.

The current implementation of clear() also handles (in a somewhat roundabout way) self-assignments:

First the _refcount is decremented, then it is incremented again (on the same Symbol*)

It might be more effective to add a more conventional assignment operator, including guards for  
self-assignment:

// Assignment operator decrements the reference count for any
// existing resource. It increments the reference count
// for the newly assigned resource.
const TempNewSymbol& operator=(const TempNewSymbol &rhs) {
  if (this != &rhs && _temp != rhs._temp) {
    clear();
    _temp = rhs._temp;
    if (_temp != NULL) {
      _temp->increment_refcount();
    }
  }
  return *this;
}

Why const TempNewSymbol& as return value? To disallow the following situation:

TempNewSymbol a, b, c;

(a = b) = c;

In addition, the clear() member function does not need to NULL out the current reference, hence:

// Decrement reference counter so it can go away if it's unique
void clear() {
  if (_temp != NULL) {
    _temp->decrement_refcount();
  }
}