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.

Sign up for GitHub

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

Jump to bottom

Fix 23785: ICE with -preview=in when passing enum at CTFE #14999

Open
wants to merge 1 commit into
base: stable
Choose a base branch
from
Open

Fix 23785: ICE with -preview=in when passing enum at CTFE #14999

wants to merge 1 commit into from
+79 −5

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented Mar 16, 2023 •
edited
Loading

No description provided.

@Geod24 Geod24 requested a review from kinke March 16, 2023 21:09
@dlang-bot
Copy link
Contributor

dlang-bot commented Mar 16, 2023 •
edited
Loading

Thanks for your pull request, @Geod24!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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

  • In preparation for migrating from Bugzilla to GitHub Issues, the issue reference syntax has changed. Please add the word "Bugzilla" to issue references. For example, Fix Bugzilla Issue 12345 or Fix Bugzilla 12345.(Reminder: the edit needs to be done in the Git commit message, not the GitHub pull request.)

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "stable + dmd#14999"

kinke
kinke reviewed Mar 16, 2023
View reviewed changes
compiler/src/dmd/expressionsem.d
ev = new CommaExp(arg.loc, ev, new VarExp(arg.loc, v));
arg = ev.expressionSemantic(sc);
eprefix = Expression.combine(
eprefix, (new DeclarationExp(arg.loc, v)).expressionSemantic(sc));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this looks like it messes with the argument expressions evaluation order - evaluating these in rvalues before all other argument expressions, regardless of their position.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Changed the fix and added a test.

@Geod24
Copy link
Member Author

Geod24 commented Mar 17, 2023

@kinke and I discussed and there's a need for some more tests/work, as e.g. temporaries are now always destroyed even if not constructed.

@dkorpel
Copy link
Contributor

dkorpel commented May 2, 2023

Is this still an improvement on its own @Geod24 ?

@RazvanN7
Copy link
Contributor

@Geod24 What's the status of this PR?

@kinke
Copy link
Contributor

kinke commented May 17, 2023

It needs work, as per the label.

@Geod24
Copy link
Member Author

Geod24 commented Sep 18, 2024

@kinke : I added a test to ensure that a non-constructed rvalues structs are not destructed

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

@kinke kinke kinke left review comments

@RazvanN7 RazvanN7 RazvanN7 approved these changes

@ljmf00 ljmf00 ljmf00 approved these changes

Assignees
No one assigned
Labels
Severity:Bug Fix stable-stalled
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Geod24 @dlang-bot @dkorpel @RazvanN7 @kinke @ljmf00