463867 - Superfluous code in ScriptableObject associateValue

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, enhancement)

Description

[Peter Michaux]

In ScriptableObject, the definition of associateValue is

public synchronized final Object associateValue(Object key, Object value)
{
    if (value == null) throw new IllegalArgumentException();
    Map<Object,Object> h = associatedValues;
    if (h == null) {
        h = associatedValues;
        if (h == null) {
            h = new HashMap<Object,Object>();
            associatedValues = h;
        }
    }
    return Kit.initHash(h, key, value);
}

This looks something like double check locking but without a synchronized check between the two tests for h being null. 

Since the whole method is synchronized, couldn't this code be reduced to

public synchronized final Object associateValue(Object key, Object value)
{
    if (value == null) throw new IllegalArgumentException();
    Map<Object,Object> h = associatedValues;
    if (h == null) {
        h = new HashMap<Object,Object>();
        associatedValues = h;
    }
    return Kit.initHash(h, key, value);
}

Comment 1

[Peter Michaux]

Attachment: changes per the ticket description

Comment 2

[Norris Boyd]

Fixed: 

Checking in src/org/mozilla/javascript/ScriptableObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptableObject.java,v  <--  ScriptableObject.java
new revision: 1.143; previous revision: 1.142
done