-
-
Notifications
You must be signed in to change notification settings - Fork 753
Fix issue #13017: opEquals for null std.typecons.Nullable #5032
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2185,6 +2185,69 @@ Params: | |
| _isNull = false; | ||
| } | ||
|
|
||
| /** | ||
| 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 | ||
| { | ||
| if (_isNull) | ||
| return rhs._isNull; | ||
| if (rhs._isNull) | ||
| return false; | ||
| return _value == rhs._value; | ||
| } | ||
|
|
||
| /// Ditto | ||
| bool opEquals()(auto ref const(T) rhs) const | ||
| { | ||
| return _isNull ? false : rhs == _value; | ||
| } | ||
|
|
||
| /// | ||
| @safe unittest | ||
| { | ||
| Nullable!int empty; | ||
| Nullable!int a = 42; | ||
| Nullable!int b = 42; | ||
| Nullable!int c = 27; | ||
|
|
||
| assert(empty == empty); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For documentation sake, I would add a line here that says |
||
| assert(empty == Nullable!int.init); | ||
| assert(empty != a); | ||
| assert(empty != b); | ||
| assert(empty != c); | ||
|
|
||
| assert(a == b); | ||
| assert(a != c); | ||
|
|
||
| assert(empty != 42); | ||
| assert(a == 42); | ||
| assert(c != 42); | ||
| } | ||
|
|
||
| @safe unittest | ||
| { | ||
| // Test constness | ||
| immutable Nullable!int a = 42; | ||
| Nullable!int b = 42; | ||
| immutable Nullable!int c = 29; | ||
| Nullable!int d = 29; | ||
| immutable e = 42; | ||
| int f = 29; | ||
| assert(a == a); | ||
| assert(a == b); | ||
| assert(a != c); | ||
| assert(a != d); | ||
| assert(a == e); | ||
| assert(a != f); | ||
|
|
||
| // Test rvalue | ||
| assert(a == const Nullable!int(42)); | ||
| assert(a != Nullable!int(29)); | ||
| } | ||
|
|
||
| template toString() | ||
| { | ||
| import std.format : FormatSpec, formatValue; | ||
|
|
@@ -2317,15 +2380,19 @@ Returns: | |
| /// | ||
| @system unittest | ||
| { | ||
| import core.exception : AssertError; | ||
| import std.exception : assertThrown, assertNotThrown; | ||
|
|
||
| Nullable!int ni; | ||
| int i = 42; | ||
| //`get` is implicitly called. Will throw | ||
| //an AssertError in non-release mode | ||
| assertThrown!Throwable(ni == 0); | ||
| assertThrown!AssertError(i = ni); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. |
||
| assert(i == 42); | ||
|
|
||
| ni = 0; | ||
| assertNotThrown!Throwable(ni == 0); | ||
| ni = 5; | ||
| assertNotThrown!AssertError(i = ni); | ||
| assert(i == 5); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of accessing a pseudo private member, why not
return _value == rhs.get;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you do that? This is
opEquals. It's normal to access the member variables of the two variables to compare them. Callinggetwould just add unnecessary overhead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you're accessing an undocumented public member, something that we're trying to move away from and make these members private.
_valueshould really beprivateorpackage.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is private. And since this is a member function, it's accessible. The fact that it's from another instance doesn't matter. The normal thing to do in a function like
opEqualsoropCmpis to use the member variables directly, and it's often the case that that's the only way that they can be used, because they're private. It would be incredibly bizarre to call a member function to access a member to compare it inopEqualsrather than just accessing it directly.