Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added opEquals to Nullable(T, T nullValue) #6583

Closed
wants to merge 6 commits into from
Closed

Added opEquals to Nullable(T, T nullValue) #6583

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e938ed0
Added opEquals to Nullable(T, T nullValue)
bausshf Jun 15, 2018
992dee1
Update typecons.d
bausshf Jun 15, 2018
e014ad4
Removed tabs
bausshf Jun 15, 2018
f973fb8
Changed unittest from @safe to @system
bausshf Jun 15, 2018
a81b9b5
Single return statement for opEquals
bausshf Jun 15, 2018
e3bb226
remove tab
schveiguy Jun 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions std/typecons.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3431,6 +3431,32 @@ Params:
assert(!npi.isNull);
}

/**
If they are both null, then they are equal. If one is null and the other
is not, then they are not equal. If they are both non-null, then they are
equal if their values are equal.
*/
bool opEquals()(auto ref const(typeof(this)) rhs) const
{
return _value == rhs._value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

}

/// Ditto
bool opEquals(U)(auto ref const(U) rhs) const
if (is(typeof(this.get == rhs)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@FeepingCreature FeepingCreature Jun 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
return isNull ? false : rhs == _value;
}

@system unittest
{
Nullable!(uint, uint.max) val3;
Nullable!(uint, uint.max) val4;
assert(val3.isNull);
assert(val4.isNull);
assert(val3 == val4);
}

/**
Gets the value. `this` must not be in the null state.
This function is also called for the implicit conversion to `T`.
Expand Down