Fix issue #13017: opEquals for null std.typecons.Nullable #5032
Conversation
|
| */ | ||
| bool opEquals()(auto ref inout(typeof(this)) rhs) inout | ||
| { | ||
| if(_isNull) |
There was a problem hiding this comment.
The code style is to have a whitespace after if (that's why CircleCi is failing)
Btw it's also not covered by the unittests.
| return rhs._isNull; | ||
| if(rhs._isNull) | ||
| return false; | ||
| return _value == rhs._value; |
There was a problem hiding this comment.
instead of accessing a pseudo private member, why not return _value == rhs.get;
There was a problem hiding this comment.
Why would you do that? This is opEquals. It's normal to access the member variables of the two variables to compare them. Calling get would just add unnecessary overhead.
There was a problem hiding this comment.
Because you're accessing an undocumented public member, something that we're trying to move away from and make these members private. _value should really be private or package.
There was a problem hiding this comment.
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 opEquals or opCmp is 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 in opEquals rather than just accessing it directly.
| Nullable!int b = 42; | ||
| Nullable!int c = 27; | ||
|
|
||
| assert(empty == empty); |
There was a problem hiding this comment.
For documentation sake, I would add a line here that says assert(empty.isNull);
| //`get` is implicitly called. Will throw | ||
| //an AssertError in non-release mode | ||
| assertThrown!Throwable(ni == 0); | ||
| assertThrown!AssertError(i = ni); |
There was a problem hiding this comment.
No. == never asserts now, whereas = does.
|
Fixed whitespace. |
|
the codecov error looks like a coverage bug |
| 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 inout(typeof(this)) rhs) inout |
There was a problem hiding this comment.
Nit: const is more appropriate than inout, since you don't depend on the mutability in any way.
I was getting sick of this not working, and there have been multiple bug reports on this, so I'm just fixing it. With these changes, using == and != on null Nullables will not result in an AssertError but instead will consider them equal if they're both null, equal if they're both non-null and have the same value, and not equal otherwise.
|
Auto-merge toggled on |
|
This pull request introduced a regression: |
|
@jmdavis ping, please look at the regression! |
I was getting sick of this not working, and there have been multiple bug
reports on this, so I'm just fixing it. With these changes, using == and
!= on null Nullables will not result in an AssertError but instead will
consider them equal if they're both null, equal if they're both non-null
and have the same value, and not equal otherwise.