Skip to content

[dmd/ctfeexpr] Don't copy ArrayLiteralExp.basis more than once - #23372

Draft
ntrel wants to merge 2 commits into
dlang:masterfrom
ntrel:ct-array-basis
Draft

[dmd/ctfeexpr] Don't copy ArrayLiteralExp.basis more than once#23372
ntrel wants to merge 2 commits into
dlang:masterfrom
ntrel:ct-array-basis

Conversation

@ntrel

@ntrel ntrel commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Fixes #23367.

@thewilsonator thewilsonator added the Review:Industry Applies to PRs pertaining to industry applications of D label Jul 6, 2026
@thewilsonator

Copy link
Copy Markdown
Contributor

Do we have a test case for this?

@dkorpel

dkorpel commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

This would be an interesting PR for the benchmark CI that @abulgit is working on.

abulgit added a commit to abulgit/dmd that referenced this pull request Jul 7, 2026
@ntrel

ntrel commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

@thewilsonator

Do we have a test case for this?

A self-contained example testing this is bug5147:
https://github.com/dlang/dmd/blob/master/compiler/test/compilable/interpret3.d#L805-L810

@ntrel

ntrel commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

A self-contained example testing this is bug5147:

Actually that one is strange, it sets basis but no elements are actually null. I think it's due to sarrayRepeat in initsem.d.

@ntrel
ntrel marked this pull request as draft July 10, 2026 13:00
@ntrel

ntrel commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

With this PR, the following crashes:

alias f = {
    int[1][] pieces = [];
    pieces.length = 2;
    return pieces;
};
pragma(msg, f()); // segfault
static assert(f() == [[0], [0]]); // segfault

@ntrel

ntrel commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

I thought this might be an easy fix, but there's just too much code that ignores basis and indexes the elements field directly.

@WalterBright

Copy link
Copy Markdown
Member

@ntrel your test case works now!

@thewilsonator thewilsonator reopened this Aug 8, 2026
@thewilsonator

Copy link
Copy Markdown
Contributor

Hmm, I might have messed up the rebase for this, the change is now trivial.

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

Labels

Review:Industry Applies to PRs pertaining to industry applications of D

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Excessive memory consumption with large array literals

4 participants