Modify

Opened 6 weeks ago

Closed 5 weeks ago

Last modified 7 days 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:

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

Why isn't field id used directly?

Attachments (1)

17440.patch (7.6 KB) - added by GerdP 6 weeks ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 6 weeks ago by Don-vip

Description: modified (diff)

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

comment:2 Changed 6 weeks 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 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.

comment:3 Changed 6 weeks ago by GerdP

Summary: Confuisng codeConfusing code

comment:4 Changed 6 weeks 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 6 weeks ago by GerdP

Attachment: 17440.patch added

comment:5 Changed 5 weeks 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 5 weeks 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 7 days ago by Don-vip

Milestone: 19.03
Type: taskenhancement

Modify Ticket

Change Properties
Set your email in Preferences
Action
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.