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 20465 - Dynamic + static array declaration fail #10698

Merged
merged 1 commit into from
Dec 28, 2019

Conversation

MoonlightSentinel
Copy link
Contributor

Implements a cleaner solution for issue 10562 which tries to match the assigned expression against all dimensions of the target (instead of simply rewriting to keep type safety).

Rewrites the original test to orderly check all possible combinations.

@dlang-bot
Copy link
Contributor

dlang-bot commented Dec 26, 2019

Thanks for your pull request and interest in making D better, @MoonlightSentinel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

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

Auto-close Bugzilla Severity Description
20465 regression Dynamic + static array declaration fail

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 "stable + dmd#10698"

src/dmd/expressionsem.d Show resolved Hide resolved
src/dmd/expressionsem.d Outdated Show resolved Hide resolved
@Geod24
Copy link
Member

Geod24 commented Dec 26, 2019

Looks like you botched the rebase.

@Geod24
Copy link
Member

Geod24 commented Dec 26, 2019

Isn't it a bit too lax tho ?We always had the ability to assign a value to a slice, but never to assign a value to a multi dimensional array. Also, does it work with int[][] ?

@MoonlightSentinel
Copy link
Contributor Author

Looks like you botched the rebase.

Fixed

Isn't it a bit too lax tho ?We always had the ability to assign a value to a slice, but never to assign a value to a multi dimensional array.

Maybe, but there was always a discrepancy between the initial construction and further usage of static arrays

int[2][3] x = 2; // Valid
x = 3;           // Invalid

Also, does it work with int[][] ?

This change applies to assignment/construction of an static array, do you have a specific example?

@Geod24
Copy link
Member

Geod24 commented Dec 26, 2019

This change applies to assignment/construction of an static array, do you have a specific example?

For starter:

int[3][] arr = new int[3][5];
arr = 42;

But you can extend it to int[][].

EDIT: Also, there's a reason why the discrepancy is possible. When you construct the array, you specify the type, while when you assign it, you don't.

@MoonlightSentinel
Copy link
Contributor Author

This change applies to assignment/construction of an static array, do you have a specific example?

For starter:

int[3][] arr = new int[3][5];
arr = 42;

But you can extend it to int[][].

These are dynamic arrays and hence unaffected by this patch.
arr = 42 cannot be rewritten as an int[15] since the length is (usually) unknown at compile time (but maybe it would be possible to track it in this case)

@MoonlightSentinel
Copy link
Contributor Author

Lets defer the discussion about the differences between construction and assignment to another PR as its not required to fix this regression (unless the test suite disagrees).

Got kinda carried away there ...

Implements a cleaner solution for issue 10562 which tries to match the
assigned expression against all dimensions of the target (instead of
bluntly rewriting to keep type safety).

Rewrites the original test to orderly check all possible combinations.
@MoonlightSentinel
Copy link
Contributor Author

MoonlightSentinel commented Dec 26, 2019

@Geod24 Restored the original restriction to non-assignments and adapted the test accordingly

@MoonlightSentinel
Copy link
Contributor Author

Can this be merged or should we revert the faulty PR until we come to a conclusion? (Since the next release is due in a few days)

@dlang-bot dlang-bot merged commit 59d46bc into dlang:stable Dec 28, 2019
@MoonlightSentinel MoonlightSentinel deleted the sa-rewrite branch December 28, 2019 13:25
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.

6 participants