Added opEquals to Nullable(T, T nullValue)

[bausshf]

Right now comparing a Nullable(T, T nullValue) with another Nullable(T, T nullValue) will cause an AssertError, unlike comparing Nullable(T) with another Nullable(T). They should both have same behavior.

[dlang-bot]

Thanks for your pull request and interest in making D better, @bausshf! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
• 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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#6583"

[quickfur]

This doesn't seem right. Shouldn't it be:

if (is(typeof(this.get) == typeof(rhs)))

?

[bausshf]

That is possible, it's taken from Nullable(T) which means the issue will exist within both types.

Will change it for both if that would be correct. Will wait for a more official answer to that.

[schveiguy]

No, it's right, it's asking can this.get() == rhs compile.

You wouldn't want the other way, because then e.g. Nullable!int couldn't be compared to 1L

[schveiguy]

I think this is correct, but it may be surprising in some cases. For example:

enum NullValue = int.max;
Nullable!(int, NullValue) n;

assert(n == NullValue); // fails

However, I also don't know that we want to make this true either. I think the proposed behavior is at least consistent with the other Nullable.

[FeepingCreature]

At least should be __traits(compiles, this.get == rhs) to avoid ambiguity, right?

[schveiguy]

Can you explain the ambiguity? I'm always lost when it comes to the differences between __traits(compiles and is(typeof

[FeepingCreature]

I mean, to avoid the confusion with is(typeof() == typeof()).

is(typeof()) is the legacy, "D1" way to do __traits(compiles).

(is() in general, and all its myriad magics and mysteries, is what __traits is supposed to replace.)

[schveiguy]

But there are subtle differences. I don't remember what they are, but someone (maybe Timon or Stefan?) has outlined why you should prefer one or the other, I don't have the link to the NG post handy. I'd prefer leaving it the way it is for this PR, since it's already that way for the other version of Nullable, and we can explore changing the is(typeof to __traits(compiles across the whole module in another PR.

[schveiguy]

I approve, just simplify the default opEquals.

[schveiguy]

This can be simplified to return _value == rhs._value

[schveiguy]

And by that I mean, the whole function can reduce to that :) I realize reading my comment may not be clear.

[bausshf]

You're entirely correct!

[schveiguy]

I think this is correct, but it may be surprising in some cases. For example:

enum NullValue = int.max;
Nullable!(int, NullValue) n;

assert(n == NullValue); // fails

However, I also don't know that we want to make this true either. I think the proposed behavior is at least consistent with the other Nullable.

[schveiguy]

Ugh, I hate to do this to you, but actually there needs to be one static if: if _value is a floating point, then this won't work, but isNull will. So you need to duplicate what isNull does in that instance.

[dukc]

Or make null == null implementation defined? If one really needs to compare nulls, let him do so manually.

[n8sh]

We can just do:

bool opEquals()(auto ref const(typeof(this)) rhs) const
{
    // We check __traits(compiles) in case opEquals is not CTFE-able.
    static if (__traits(compiles, { static assert(nullValue == nullValue); }))
        return _value == rhs._value;
    else
        // Note that it is wrong to use this branch for floating point T
        // when nullValue is not NaN.
        return _value is rhs._value;
}

[schveiguy]

This should not be necessary. And in fact, there are other places it's @safe. Something is wrong here, it appears the compiler isn't properly inferring safety.

[schveiguy]

At line 3483, you need to change the assertThrown call. This is a documented unittest, so I'd suggest:

    // even though -1 is the null value, comparing with any int is false
    // because ni.isNull is true.
    assert(ni != -1);

[schveiguy]

I will approve once the new concerns are addressed. Still needs a bit of work.

[schveiguy]

OK, doing some digging, it appears there is a compiler error causing it to give up inferring attributes for some reason. We may not be able to fix this without a compiler change. Problem is, I don't know why it's not doing the inference, so it may be hard to make a reduced case.

[schveiguy]

The compiler bug has something to do with a circular dependency. The compiler is thinking it can call the second form of opEquals with typeof(this).

I was able to silence the error by changing the template constraint:

    bool opEquals(U)(auto ref const(U) rhs) const
    if (!is(typeof(rhs) == const(typeof(this))) && is(typeof(this.get == rhs)))

I'm not sure why it's not required for the version that doesn't take a nullable sentinel value.

[bausshf]

Sorry, I have been a lot busy with a lot other stuff, but I will get back to the rest of the modifications needed later today since I got some time on my hand.

[RazvanN7]

This has been fixed in master. The unittest added in this PR runs succesfully.