recent coding style changes

[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.

[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. :)

[Don-vip]

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 :) )

[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.

[Don-vip]

In 8393/josm:

see #11447 - partial revert of r8384

[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)

[Don-vip]

In 8416/josm:

fix #11447 - revert or r8384 for StyleCache and GeoPropertyIndex