Skip to content

Conversation

sugmanue
Copy link
Contributor

For primitive floating-point members, the existing logic would render the equals method using ==. This change uses instead Float.compare as it gives better results. E.g., for the added test model now the equals method looks like:

    @Override
    public boolean equals(Object other) {
        if (other == this) {
            return true;
        }
        if (other == null || getClass() != other.getClass()) {
            return false;
        }
        StructureOne that = (StructureOne) other;
        return this.byteMember == that.byteMember
                && this.shortMember == that.shortMember
                && this.intMember == that.intMember
                && this.longMember == that.longMember
                && Float.compare(this.floatMember, that.floatMember) == 0
                && Double.compare(this.doubleMember, that.doubleMember) == 0
                && this.booleanMember == that.booleanMember;
    }

Also add a test runner to assert against expected generated java files. If the approach seems fine I can add more tests to the types codegen plugin and the client and server.

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Also add a test runner to assert against expected generated java files.
@rhernandez35
Copy link
Contributor

"Better" is relative here. == considers all NaN representations as different and compare considers all NaN representations as equivalent.

jshell> Float.NaN == Float.NaN
$1 ==> false

jshell> Float.NaN + 1 == Float.NaN
$2 ==> false

jshell> Float.compare(Float.NaN, Float.NaN) == 0
$3 ==> true

jshell> Float.compare(Float.NaN + 1, Float.NaN) == 0
$4 ==> true

Smithy defines floating point values as IEEE-754 floats and the Java Language Specification defines specific comparison behavior for such values, in accordance with IEEE-754:

Floating-point equality testing is performed in accordance with the rules of the IEEE 754 standard:

If either operand is NaN, then the result of == is false but the result of != is true.

Merging this change will cause our comparison behavior to differ from both IEEE-754 and the JLS. I do not think we should proceed with it.

@sugmanue
Copy link
Contributor Author

I guess the question is what's the equals method for. If we were working on a number crunching library I would agree with you, however, I feel like the users of libraries like this one will benefit a lot more from a behavior that mirrors intuition. Strict adherence to IEEE-754 starts as surprising and ends up being just an annoyance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants