Opened 16 months ago

Closed 16 months ago

Last modified 15 months ago

#17440 closed enhancement (fixed)

Confusing code

Reported by: GerdP Owned by: team
Priority: normal Milestone: 19.03
Component: Core Version:
Keywords: Cc:

Description (last modified by Don-vip)

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

    public long getId() {
        long id =;
        return id >= 0 ? id : 0;

Why isn't field id used directly?

Attachments (1)

17440.patch (7.6 KB) - added by GerdP 16 months ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 16 months ago by Don-vip

Description: modified (diff)

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

comment:2 Changed 16 months ago by 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 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:

    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.

comment:3 Changed 16 months ago by GerdP

Summary: Confuisng codeConfusing code

comment:4 Changed 16 months ago by 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.

Changed 16 months ago by GerdP

Attachment: 17440.patch added

comment:5 Changed 16 months ago by GerdP

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

comment:6 Changed 16 months ago by GerdP

Resolution: fixed
Status: newclosed

In 14905/josm:

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

comment:7 Changed 15 months ago by Don-vip

Milestone: 19.03
Type: taskenhancement

Modify Ticket

Change Properties
Set your email in Preferences
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment

E-mail address and name can be saved in the Preferences.

Note: See TracTickets for help on using tickets.