Confusing code

[GerdP]

I see quite a few methods with a construct like this in AbstractPrimitive:

    @Override
    public long getId() {
        long id = this.id;
        return id >= 0 ? id : 0;
    }

Why isn't field id used directly?

[Don-vip]

Added in r3348. Not sure if it's really needed.

[GerdP]

The comment says the change is about thread safety. I don't think that this code makes getId() threadsafe. If I got that right the idea is that this.id might be changed when

    return id >= 0 ? id : 0;

is executed. Not sure if that is possible but if it is we might still return a wrong result.
Same here:

    @Override
    public final int getNumKeys() {
        String[] keys = this.keys;
        return keys == null ? 0 : keys.length / 2;
    }

If this.keys is changed from or to null after the local variable was set we would return a wrong result and the caller has no way to catch this situation.
I think the code is a very bad idea when it comes to thread safety.
Question is if we need correct synchronization or if we can simply remove the assignment to the local variable.

[GerdP]

Sonar lint shows 83 issues for rule "Local variables should not shadow class fields". Many of them are because of this trick. I'll try to identify those "trick" lines and create a patch that removes them.

[GerdP]

I've now worked with this patch for a while and found no problems.
If nobody objects I'll commit this patch tomorrow.

[GerdP]

In 14905/josm:

fix #17440: remove confusing code
fixes several sonar lint issues "Local variables should not shadow class fields"