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 6208 with tuple parameter deduction in IFTI. #709
Conversation
if (m < match) | ||
match = m; | ||
|
||
t->objects.tdata()[i] = tt; | ||
} |
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.
Can't this be done with four ifs and an else? I think it would be clearer if the code expressed the rule being followed, instead of listing every single case.
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.
The way listing each cases is same as Type::deduceType()
. I think we need to refactor it, but it isn't a purpose of this pull.
But, default: assert(0);
is bad. It will cause unintended ICE. I'll fix it.
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.
Ah, I didn't realize. I actually think having an assert(0)
is the right way to go here, every case is covered, so the only way it will be hit is if a new modifier is added, and the assert(0)
will cause the bug to be caught faster rather than waiting for someone to realize that the new combination is never matched.
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.
Sorry, I had had silly mistake.
m = MATCHnomatch;
break; /* was lacked!! */
default:
//m = MATCHnomatch;
//break;
assert(0); // never comming is right
}
LGTM :) Thanks for the quick fix! |
assert(s.bar(10) == 1); | ||
assert(s.baz(10) == 2); | ||
assert(s.war(10) == 1); | ||
} |
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.
Can we add more test cases for lvalues and const?
Not sure if I got them right.
int n = 10;
assert(s.foo(n) == 2); // not sure about lvalue and auto ref
assert(s.hoo(n) == 1);
assert(s.bar(n) == 1);
assert(s.baz(n) == 2);
assert(s.war(n) == 1);
assert(s.foo(cast(const)10) == 1);
assert(s.hoo(cast(const)10) == 1);
assert(s.bar(cast(const)10) == 2);
assert(s.baz(cast(const)10) == 1);
assert(s.war(cast(const)10) == 1);
const int cn = 10;
assert(s.foo(cn) == 2); // not sure about lvalue and auto ref
assert(s.hoo(cn) == 1);
assert(s.bar(cn) == 2);
assert(s.baz(cn) == 1);
assert(s.war(cn) == 1);
assert(s.foo(cast(immutable)10) == 1);
assert(s.hoo(cast(immutable)10) == 1);
assert(s.bar(cast(immutable)10) == 2);
assert(s.baz(cast(immutable)10) == 1);
assert(s.war(cast(immutable)10) == 1);
immutable int in = 10;
assert(s.foo(in) == 2); // not sure about lvalue and auto ref
assert(s.hoo(in) == 1);
assert(s.bar(in) == 2);
assert(s.baz(in) == 1);
assert(s.war(in) == 1);
Probably testing free functions makes sense too.
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.
OK, Added tests for mutable/const/immutable arguments.
If a function parameter using tuple template parameter has some storage classes, the its whole matching level should be the worst of each tuple elements.
fix Issue 6208 with tuple parameter deduction in IFTI.
Fix unnecessary template bloat in enforce.
If a function parameter using tuple template parameter has some storage classes, the its whole matching level should be the worst of each tuple elements.