Make new AA allocate the associative array Impl#14257
Conversation
|
Thanks for your pull request and interest in making D better, @ntrel! 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 run digger -- build "master + dmd#14257" |
ntrel
changed the title
new on an associative array allocate the AA Implnew AA allocate the associative array Impl
|
I think you need to |
|
Please rebase |
|
is this good to go? |
|
@thewilsonator Yes, now marked as ready. |
| // e.g. `new Alias(args)` | ||
| if (nargs) | ||
| { | ||
| exp.error("`new` cannot take arguments for an associative array"); |
There was a problem hiding this comment.
Please add a test for this
| auto b = a; | ||
| ... | ||
| a["seven"] = 7; | ||
| assert(b["seven"] == 7); |
There was a problem hiding this comment.
This would be a good addition to the runnable test
| } | ||
| e = el_combine(ezprefix, e); | ||
| } | ||
| else if (auto taa = t.isTypeAArray()) |
There was a problem hiding this comment.
I don't understand why Codecov says this isn't covered, it has to be or else the runnable test wouldn't work right?
4 participants
Fixes Issue 10535.
https://issues.dlang.org/show_bug.cgi?id=10535.
Requires dlang/druntime#3863. See description there for justification.