Modify

Opened 4 years ago

Closed 4 years ago

#11447 closed defect (fixed)

recent coding style changes

Reported by: bastiK Owned by: team
Priority: normal Milestone: 15.05
Component: Core Version:
Keywords: Cc:

Description

A lot of work has been done recently to improve the overall code quality by using automatic code scanning tools. The vast majority of those changes I fully support, but there are some minor things I would like to discuss / reconsider.

[8384]:

  • It is wrong to use Utils.equalsEpsilon in @Override equals as it violates the principle that equal objects must have the same hash code (e.g. in LineElemStyle)
  • It is not always an error to use "==" for floating point comparison, sometimes it is intended. For example in StyleCache, with equalsEpsilon it becomes a gamble that the code works correctly. I could construct a corner case example where it would throw an AssertionError. (similar problem in GeoPropertyIndex, I think)
  • Why do you change x == 0.0 to Double.doubleToRawLongBits(x) == 0? First of all, it looks silly.
    The performance improvement (if any) should be negligible. Next, it also changes the semantics. E.g. in ExpressionFactory.Functions.divided_by(x, ..., y), before dividing x / y, it checks if y is 0.0 or -0.0. Now it only checks 0.0. (same problem in Geometry)

I can fix these things myself, just like to give a chance to comment. :)

Attachments (0)

Change History (5)

comment:1 in reply to:  description ; Changed 4 years ago by Don-vip

Milestone: 15.05

Replying to bastiK:

A lot of work has been done recently to improve the overall code quality by using automatic code scanning tools. The vast majority of those changes I fully support, but there are some minor things I would like to discuss / reconsider.

thanks :)

[8384]:

  • It is wrong to use Utils.equalsEpsilon in @Override equals as it violates the principle that equal objects must have the same hash code (e.g. in LineElemStyle)

You're right, I'm going to revert that.

  • It is not always an error to use "==" for floating point comparison, sometimes it is intended. For example in StyleCache, with equalsEpsilon it becomes a gamble that the code works correctly. I could construct a corner case example where it would throw an AssertionError. (similar problem in GeoPropertyIndex, I think)

Can you please revert the locations where this is problematic? even if you're not completely sure :)

  • Why do you change x == 0.0 to Double.doubleToRawLongBits(x) == 0? First of all, it looks silly.
    The performance improvement (if any) should be negligible. Next, it also changes the semantics. E.g. in ExpressionFactory.Functions.divided_by(x, ..., y), before dividing x / y, it checks if y is 0.0 or -0.0. Now it only checks 0.0. (same problem in Geometry)

I maybe overlooked the bug description:
http://josm.openstreetmap.de/sonar/coding_rules#rule_key=squid%3AS1244

I assumed it would ensure correctness at no performance cost, how do you understand this rule?

I can fix these things myself, just like to give a chance to comment. :)

Thanks :) I'll discuss upcoming changes that could be problematic before committing now (should have done that from the beginning :) )

comment:2 in reply to:  1 Changed 4 years ago by bastiK

Replying to Don-vip:

Replying to bastiK:

  • Why do you change x == 0.0 to Double.doubleToRawLongBits(x) == 0? First of all, it looks silly.
    The performance improvement (if any) should be negligible. Next, it also changes the semantics. E.g. in ExpressionFactory.Functions.divided_by(x, ..., y), before dividing x / y, it checks if y is 0.0 or -0.0. Now it only checks 0.0. (same problem in Geometry)

I maybe overlooked the bug description:
http://josm.openstreetmap.de/sonar/coding_rules#rule_key=squid%3AS1244

I assumed it would ensure correctness at no performance cost, how do you understand this rule?

This part of the rule, I don't understand.

x == 0 is equivalent to Double.doubleToRawLongBits(x) == 0 || Double.doubleToRawLongBits(x) == 0x8000000000000000L (where 0x8000000000000000L is the bit pattern for negative zero). I'm not sure, why it should be an improvement to skip the second check. Maybe if someone is using it wrong, then it will be more obvious that it is wrong.

When 0.0 is a magic value for "not initialized", then it does not matter (both works). But for checks like division by zero, "x == 0" makes more sense to me.

comment:3 Changed 4 years ago by Don-vip

In 8393/josm:

see #11447 - partial revert of r8384

comment:4 Changed 4 years ago by Don-vip

In 8399/josm:

see #11397, see #11447 - partial revert of r8308 (SONARJAVA-1061: FP on S1948: no issue should be raised when using Collections of Serializable objects)

comment:5 Changed 4 years ago by Don-vip

Resolution: fixed
Status: newclosed

In 8416/josm:

fix #11447 - revert or r8384 for StyleCache and GeoPropertyIndex

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.