Fix Issue 13663 - Comparison of Tuples with floating point fields

[ghost]

Thanks to Simen Kjaeraas for pointing out this solution.

[dlang-bot]

Thanks for your pull request and interest in making D better, @berni44! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	13663	normal	Comparison of Tuples with floating point fields

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#7301"

[ghost]

Two of the tests in buildkite produce this error:

std/typecons.d(814,32): Error: template instance isFloatingPoint!"key" does not match template declaration isFloatingPoint(T)

It's clear, that this comes from the tuple having named elements. But it seems like I need a more thorough understanding of how Tuple works for getting this right.

[ghost]

I switched to math:isNaN, because now we know, that we compare floats.

And: It feels a little bit strange to me, that the two functions differ only by a "const"...

[ghost]

I just noticed, that the docs of opCmp where wrong too and obviously did not account for NaNs. I fixed that too.

[Biotronic]

Suggest adding this for user-defined types with float opCmps:

static if (isFloatingPoint!(typeof(field[i].opCmp(field[i]))))
{
    if (field[i] != field[i])
        return float.nan;
}
static if (isFloatingPoint!(typeof(rhs.field[i].opCmp(rhs.field[i]))))
{
    if (rhs.field[i] != rhs.field[i])
        return float.nan;
}

[Biotronic]

Bah. The above fails since of course float doesn't have an opCmp member. Something like this is required:
static if (is(typeof({static assert(isFloatingPoint!(typeof(field[i].opCmp(field[i]))));})))
For the record: that's ugly.

[ghost]

What about that:

                    auto tmp1 = field[i] < rhs.field[i];
                    auto tmp2 = field[i] > rhs.field[i];
                    if (!tmp1 && !tmp2)
                        return float.nan;

[Biotronic]

That works, but brings us back to the performance discussion from earlier.

[Biotronic]

Also, kinda obsoletes the isFloatingPoint test above.

[ghost]

OK. So we need something else here...

[Biotronic]

This looks better. One very minor nitpick is it's possible to have opCmp return a type with an overloaded opCmp that returns float (and actually any number of levels to this opCmp tower). I'm not sure why anyone would do that, but as a serial offender on operator overload abuse, I feel it's my duty to inform people of its dangers. Should we want to actually support this, I would suggest adding this to std.traits:

template hasNanComparison(T1, T2 = T1)
{
    static if (isFloatingPoint!T1)
        enum hasNanComparison = true;
    else static if (!isAggregateType!T1)
        enum hasNanComparison = false;
    else static if (!is(typeof((T1 a, T2 b) => a.opCmp(b))))
        enum hasNanComparison = false;
    else
        enum hasNanComparison = hasNanComparison!(ReturnType!((T1 a, T2 b) => a.opCmp(b)));
}

unittest {
    assert( hasNanComparison!float);
    assert( hasNanComparison!real);
    assert(!hasNanComparison!int);
    assert(!hasNanComparison!string);
    assert(!hasNanComparison!(int*));
    assert(!hasNanComparison!(int[]));
    
    struct S
    {
        float opCmp(S s) { return float.nan; }
    }
    assert( hasNanComparison!S);
    
    struct S2
    {
        S opCmp(S2 s) { return S(); }
    }
    assert( hasNanComparison!S2);
}

I would argue, however, that this addition is not necessary.

[ghost]

Maybe this could be done in a separate PR if anyone needs it?!? I would be happy to finish with this PR, though.

[Biotronic]

Agreed.

[ghost]

Next try. ;-)

[RazvanN7]

Closing this in favor of: #7748