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.

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

typeMerge should take modifiers into account when searching for common base class #5450

Merged
merged 1 commit into from Feb 13, 2016

Conversation

quickfur
Copy link
Member

Fixes issue 15638.

The problem appears to be that typeMerge does not copy modifiers over when searching for a common base class, so it tries to test implicit convertibility from const(derivedClass) to (non-const) BaseClass, which fails, thus it fails to find the common base class as const(BaseClass).

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
15638 no common type for const classes

@PetarKirov
Copy link
Member

Looks good. Maybe it's also a good idea to test:

B mb;
C mc;
const B cb
const C cc;
immutable B ib;
immutable C ic;

auto a1 = true? mb : mc; // const A
auto a2 = true? cb : ic;  // const A
auto a3 = true? mb : ic; // const A
// ...

const(A)[] arr1 = new B[10];
const(A)[] arr2 = new const(B)[10];
const(A)[] arr3 = new immutable(B)[10];
// ...

// I'm not sure if this should work:
const(A)[] concat1 = new const(A)[] ~ new immutable(C)[];
//...

@quickfur
Copy link
Member Author

Good point, the case of x ? const(B) : immutable(C) still does not work. Will look into it more.
Edit: Actually, it does work; I just botched the test code. Will add a few more test cases and push.

@quickfur
Copy link
Member Author

Found that:

const(A)[] concat1 = new const(A)[] ~ new immutable(C)[];

does not work, but this does:

const(A)[] concat1 = new const(A)[1] ~ new immutable(C)[1];

I think I'll leave it out of the testcase, though, I'm not sure either whether or not it should be allowed.

@quickfur
Copy link
Member Author

Rebased and updated with more tests.

@yebblies
Copy link
Member

Needs the array tests too. And the test cases should all be crammed into xtest46 or one of the other existing megatests.

@PetarKirov
Copy link
Member

@quickfur of course this:

const(A)[] concat1 = new const(A)[] ~ new immutable(C)[];

doesnt't work! I forgot to specifying the size :D

@9rnsr
Copy link
Contributor

9rnsr commented Feb 13, 2016

LGTM.

@9rnsr
Copy link
Contributor

9rnsr commented Feb 13, 2016

Auto-merge toggled on

@yebblies
Copy link
Member

Auto-merge toggled off

@yebblies
Copy link
Member

@9rnsr This still needs the test case from the forum added (array literal type deduction) and the test should be moved out of its own file.

@quickfur
Copy link
Member Author

@yebblies Which forum thread was that? I must have missed it. Or are you referring to the array append test case posted above?

…n base class.

Fixes issue 15638.

Add test case to test suite.
@quickfur
Copy link
Member Author

Anyway, cooked up a few examples of my own. Would this do?

@yebblies
Copy link
Member

I meant this thread

I think this is covered now.

@yebblies
Copy link
Member

Auto-merge toggled on

yebblies added a commit that referenced this pull request Feb 13, 2016
typeMerge should take modifiers into account when searching for common base class
@yebblies yebblies merged commit c2c0362 into dlang:master Feb 13, 2016
@quickfur quickfur deleted the issue15638 branch February 13, 2016 19:29
@quickfur
Copy link
Member Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants