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

fix Issue 19447 - [REG2.066] fixed size slice assignment in ctfe lose… #9071

Merged
merged 1 commit into from
Dec 14, 2018

Conversation

WalterBright
Copy link
Member

…s connection with array

The difficulty was that the interpreter failed to recognize a slice of a static array as being an lvalue.

@dlang-bot
Copy link
Contributor

dlang-bot commented Dec 12, 2018

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
19447 regression [REG2.066] fixed size slice assignment in ctfe loses connection with array

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

To target stable perform these two steps:

  1. Rebase your branch to upstream/stable:
git rebase --onto upstream/stable upstream/master
  1. Change the base branch of your PR to stable

Testing this PR locally

If 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 + dmd#9071"

@WalterBright
Copy link
Member Author

It's a fairly serious bug, as it silently generates the wrong answer.

@WalterBright WalterBright force-pushed the fix19447 branch 5 times, most recently from 2358490 to 8c87322 Compare December 12, 2018 10:09
@thewilsonator
Copy link
Contributor

Please target stable. cc @UplinkCoder

@WalterBright
Copy link
Member Author

I don't think there's much point to targeting stable. This broke in version 2.066, and we're at 2.083 now.

@UplinkCoder
Copy link
Member

I'd remark that there is a bit of code duplication,
other than that, LGTM.

@thewilsonator
Copy link
Contributor

I don't think there's much point to targeting stable.

It does mean LDC get the fixes sooner.

@UplinkCoder
Copy link
Member

UplinkCoder commented Dec 13, 2018

Please add another test.
To ensure this fails:

int [2] mh = [1, 2];
void g19447(ref int[2] a)
{
    int[2] b=2;
    a=b;
    assert(a[0]==2);
}
g19447(mh);

@WalterBright
Copy link
Member Author

@UplinkCoder why should that fail?

@WalterBright WalterBright changed the base branch from master to stable December 13, 2018 10:54
@jacob-carlborg
Copy link
Contributor

Why is there an extra commit "Merge branch 'stable' into fix19447" ?

@UplinkCoder
Copy link
Member

the code I posted changes an array at global scope;
And then ctfe would modify an exsiting ast node.

@WalterBright
Copy link
Member Author

I'm putting this back into master, as it relies on some of the refactoring work I've done earlier.

@WalterBright WalterBright changed the base branch from stable to master December 13, 2018 23:20
@WalterBright
Copy link
Member Author

@UplinkCoder ok, but I had to rework it a bit, as the example is incomplete.

@WalterBright WalterBright merged commit 6064ba8 into dlang:master Dec 14, 2018
@WalterBright WalterBright deleted the fix19447 branch December 14, 2018 00:12
@ibuclaw
Copy link
Member

ibuclaw commented Dec 17, 2018

It does mean LDC get the fixes sooner.

Targeting stable however means GDC gets the fix later. :-)

@PetarKirov
Copy link
Member

@ibuclaw not necessarily much later, as you can merge stable to master at any time.

@RazvanN7
Copy link
Contributor

This PR has introduced a regression: https://issues.dlang.org/show_bug.cgi?id=20133

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

Successfully merging this pull request may close these issues.

8 participants