Bug 571191 - [AutoRefactor immigration #63/151] [cleanup & saveaction] Use Double.compare() or Float.compare()
Summary: [AutoRefactor immigration #63/151] [cleanup & saveaction] Use Double.compare(...
Status: ASSIGNED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.18   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: Fabrice Tiercelin CLA
QA Contact: Carsten Hammer CLA
URL:
Whiteboard:
Keywords: investigate
Depends on:
Blocks:
 
Reported: 2021-02-14 09:54 EST by Fabrice Tiercelin CLA
Modified: 2021-03-22 03:36 EDT (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Fabrice Tiercelin CLA 2021-02-14 09:54:12 EST
Replace arithmetic comparison by Double.compare(). Arithmetic comparison leaks precision and may lead to errors. Beware! The behavior may change if your code run with a bug!

Given:
b = primitiveDouble1 == primitiveDouble2;
b = primitiveDouble1 != primitiveDouble2;
b = primitiveDouble1 < primitiveDouble2;
b = primitiveDouble1 > primitiveDouble2;
b = primitiveDouble1 <= primitiveDouble2;
b = primitiveDouble1 >= primitiveDouble2;

When:
Clean up the code enabling "Use Double::compare"

Then:
b = Double.compare(primitiveDouble1, primitiveDouble2) == 0;
b = Double.compare(primitiveDouble1, primitiveDouble2) != 0;
b = Double.compare(primitiveDouble1, primitiveDouble2) < 0;
b = Double.compare(primitiveDouble1, primitiveDouble2) > 0;
b = Double.compare(primitiveDouble1, primitiveDouble2) <= 0;
b = Double.compare(primitiveDouble1, primitiveDouble2) >= 0;

This feature is a part of the AutoRefactor plugin immigration into Eclipse
+--------------------------------------------------------------------+-----+
| Rule in AutoRefactor                                               |     |
+--------------------------------------------------------------------+-----+
| Add brackets to control statement                                  | OK  |
| All in one method rather than loop                                 | OK  |
| Arrays.fill() rather than loop                                     | OK  |
| Assign rather than control workflow then assign anyway             | OK  |
| Atomic object rather than mono index array                         | OK  |
| AutoBoxing rather than explicit method                             | OK  |
| Break rather than passive loops                                    | OK  |
| Collapse if statements                                             | OK  |
| Comparison to 0 rather than 1 or -1                                | OK  |
| Inited collection rather than new collection and Collection.add... | OK  |
| Diamond operator                                                   | OK  |
| Double negation                                                    | OK  |
| End of method rather than return                                   | OK  |
| Equals nullable                                                    | OK  |
| Extract common code in if else statement                           | OK  |
| if-elseif                                                          | OK  |
| If rather than while and falls through                             | OK  |
| Improve lambda expressions                                         | OK  |
| Inited map rather than new map and Map.putAll()                    | OK  |
| Java 7 hash rather than Eclipse Java 6 hash                        | OK  |
| Lambda expression rather than comparator                           | OK  |
| Lazy logical rather than eager                                     | OK  |
| Literal rather than boolean constant                               | OK  |
| Local variable rather than field                                   | OK  |
| Make inner class static if it doesn't use top level class members  | OK  |
| Method on map rather than method on keyset                         | OK  |
| Merge conditional statements                                       | OK  |
| Moves increment or decrement outside an expression when possible   | OK  |
| Multi-catch                                                        | OK  |
| No assignment in if condition                                      | OK  |
| Number suffix in uppercase                                         | OK  |
| Objects equals rather than equals and null check                   | OK  |
| One code that falls through rather than redundant blocks           | OK  |
| One condition rather than unreachable block                        | OK  |
| Parsing rather than valueOf()                                      | OK  |
| Precompiles the regular expressions                                | OK  |
| Primitive comparison rather than wrapper comparison                | OK  |
| Push negation down                                                 | OK  |
| Reduce indentation                                                 | OK  |
| Remove overridden assignment                                       | OK  |
| Remove semi-colons                                                 | OK  |
| Remove super() call in constructor                                 | OK  |
| Remove unnecessary casts                                           | OK  |
| Remove unneeded this expressions                                   | OK  |
| Remove useless lone continue at the end of a loop                  | OK  |
| Remove useless modifiers                                           | OK  |
| Replace String concatenation by StringBuilder when possible        | OK  |
| Serialize rather than boxing and serialize                         | OK  |
| Simple name rather than qualified name                             | OK  |
| String rather than new string                                      | OK  |
| String.join() rather than loop                                     | OK  |
| substring() with one parameter rather than two                     | OK  |
| Switch                                                             | OK  |
| Ternary operator rather than duplicate conditions                  | OK  |
| Unboxing rather than explicit method                               | OK  |
| Use try-with-resource                                              | OK  |
| XOR rather than duplicate conditions                               | OK  |
| Double compare rather than equality                                | ... |
| Equals on constant rather than on variable                         | ... |
| If rather than two switch cases                                    | ... |
| instanceof rather than isInstance()                                | ... |
| Operand factorization                                              | ... |
| Add underscore for each thousand in number literals when it is...  | ko  |
| Aggregate constructor rather than GWT method                       | ko  |
| Android ViewHolder                                                 | ko  |
| Android WakeLock                                                   | ko  |
| Annotation                                                         | ko  |
| ArrayDeque rather than Stack                                       | ko  |
| ArrayList rather than LinkedList                                   | ko  |
| ArrayList rather than Vector                                       | ko  |
| AssertJ                                                            | ko  |
| Assign rather than ternary filter then assign anyway               | ko  |
| Big number                                                         | ko  |
| Boolean                                                            | ko  |
| Boolean constant rather than valueOf()                             | ko  |
| Boolean equals() rather than null check                            | ko  |
| Boolean primitive rather than wrapper                              | ko  |
| Brackets rather than array instantiation                           | ko  |
| Byte primitive rather than wrapper                                 | ko  |
| Char primitive rather than wrapper                                 | ko  |
| Collection.addAll() rather than list creation                      | ko  |
| Collection.contains() rather than loop                             | ko  |
| Collection.containsAll() rather than loop                          | ko  |
| Collections APIs rather than Vector pre-Collections APIs           | ko  |
| Comments                                                           | ko  |
| Comparison rather than equals                                      | ko  |
| Declaration outside loop rather than inside                        | ko  |
| Do/while rather than duplicate code                                | ko  |
| Do/while rather than while                                         | ko  |
| Double primitive rather than wrapper                               | ko  |
| Else rather than opposite condition                                | ko  |
| Empty test rather than size                                        | ko  |
| EnumMap rather than HashMap for enum keys                          | ko  |
| EnumSet rather than HashSet for enum types                         | ko  |
| Float primitive rather than wrapper                                | ko  |
| Generic list rather than raw list                                  | ko  |
| Generic map rather than raw map                                    | ko  |
| HashMap rather than Hashtable                                      | ko  |
| HashMap rather than TreeMap                                        | ko  |
| HashSet rather than TreeSet                                        | ko  |
| Implicit default constructor rather than written one               | ko  |
| Inline code rather than peremptory condition                       | ko  |
| Int primitive rather than wrapper                                  | ko  |
| JUnit asserts                                                      | ko  |
| Jupiter asserts                                                    | ko  |
| Log parameters rather than log message                             | ko  |
| Long primitive rather than wrapper                                 | ko  |
| Map.entrySet() rather than Map.keySet() and value search           | ko  |
| Move common inner if statement from then/else clauses around ou... | ko  |
| Named method rather than log level parameter                       | ko  |
| No loop iteration rather than empty check                          | ko  |
| One if rather than duplicate blocks that fall through              | ko  |
| One try rather than two                                            | ko  |
| Opposite comparison rather than negative expression                | ko  |
| Opposite condition rather than duplicate condition                 | ko  |
| OR condition rather than redundant clauses                         | ko  |
| Refactors a true or a false assertion with respectively an AND     | ko  |
| Redundant boolean                                                  | ko  |
| Redundant truth                                                    | ko  |
| Remove empty if                                                    | ko  |
| Remove empty lines                                                 | ko  |
| Remove fields default values                                       | ko  |
| Remove parenthesis                                                 | ko  |
| Remove unchecked exceptions from throws clause                     | ko  |
| Remove unnecessary local before return                             | ko  |
| Remove useless block                                               | ko  |
| Remove useless empty check before a for loop                       | ko  |
| Remove empty statements                                            | ko  |
| Replace for loop with Collections.disjoint(Collection, Collection) | ko  |
| Set rather than List                                               | ko  |
| Set rather than Map                                                | ko  |
| Short primitive rather than wrapper                                | ko  |
| Single declarations rather than multi declaration                  | ko  |
| Standard method rather than Library method                         | ko  |
| Static constant rather than instance constant                      | ko  |
| String                                                             | ko  |
| String concatenation                                               | ko  |
| String.valueOf() rather than concatenation                         | ko  |
| StringBuilder method call rather than reassignment                 | ko  |
| StringBuilder rather than StringBuffer                             | ko  |
| Super call rather than useless overriding                          | ko  |
| TestNG asserts                                                     | ko  |
| Truncating appending rather than sub-characters                    | ko  |
| Update set rather than testing first                               | ko  |
| Use java.nio.* classes instead of java.io.* classes                | ko  |
| Use String.contains()                                              | ko  |
| valueOf() rather than instantiation                                | ko  |
| Variable inside if rather than above                               | ko  |
| While condition rather than inner if                               | ko  |
+--------------------------------------------------------------------+-----+
Comment 1 Eclipse Genie CLA 2021-02-14 09:56:19 EST
New Gerrit change created: https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/176243
Comment 2 Jay Arthanareeswaran CLA 2021-03-22 01:20:58 EDT
Can you tell me why we are doing this? Consider a simple example:
 if (Double.compare(constantValue, -0.0) == 0)

The new code produces identical byte-code as the old one except that dcmpl is replaced by invokestatic.

I can't think of a reason why we should be spending time on this. If anything, we are introducing more machine cycles.

On one hand, we are trying to improve performance and then we have changes like these which is trying to prevent bugs in a code that's been around tens of hears even at a cost of a performance.
Comment 3 Fabrice Tiercelin CLA 2021-03-22 03:36:19 EDT
I'm wondering myself.