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
Fix Issue 18142 - checkedint opOpAssign doesn't accept a checkedint #6516
Conversation
Thanks for your pull request and interest in making D better, @johnsilver97! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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
Testing this PR locallyIf 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#6516" |
c06abe2
to
e10c71d
Compare
Please, don't forget to test the changes locally before opening a PR. Sometimes it's possible to miss problems related to a specific platform but here it seems that you should have seen the warning, even with |
509fe3f
to
940c649
Compare
Sorry about that, there was a case when |
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.
Thanks a lot for fixing this!
A few nits.
auto x3 = Checked!(Checked!int)(10); | ||
x1 += x3; | ||
assert(x1.get == 30); | ||
} |
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.
Sadly this needs to be outside of the template as otherwise it would be instantiated for every template.
That's the reason for static if (is(T == int) && is(Hook == void)) unittest
btw.
Also we typically add a link to the respective bugzilla issue like:
// https://issues.dlang.org/show_bug.cgi?id=18142
unittest
...
} | ||
else | ||
{ | ||
alias R = typeof(get + rhs); | ||
auto r = opBinary!op(rhs).get; | ||
import std.conv : unsigned; | ||
|
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.
Unrelated change. Please avoid.
static if (hasMember!(Hook, "hookOpOpAssign")) | ||
static assert(is(typeof(mixin("payload" ~ op ~ "=rhs")) == T) || | ||
is(typeof(Checked!(Rhs, Hook)(rhs)))); | ||
static if (is(Rhs == Checked!(X, Y), X, Y)) |
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 let the compiler do the infersion again?
You already know that its Checked!(Rhs, Hook)
, no?
auto x1 = Checked!int(10); | ||
auto x2 = Checked!int(10); | ||
x1 += x2; | ||
assert(x1.get == 20); |
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.
.get
isn't necessary here.
assert(x1.get == 20); | ||
auto x3 = Checked!(Checked!int)(10); | ||
x1 += x3; | ||
assert(x1.get == 30); |
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.
Would be great to add two more tests:
- different hook
- different type, but one that coerces to int (e.g.
short
)
The issue has been fixed by: #6685 |
I added a template constraint to
opOpAssign
so that now can accept aChecked
type asRhs
. Secondly, ifRhs
is of typeChecked
, I callopOpAssign
recursively withrhs.get
, so that I can get to the value thatChecked
wraps up.