Skip to content

Reimplement destroy as an interface #5678

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

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

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Jun 16, 2025

This changes Destroy to use an interface for its implementation.

Note that this change includes a lot of test updates. Even when Destroy is a no-op, it still causes code generation as part of determining that.

Originally I was trying to use ranges to cut down the scope of this, and to a degree I think they have. But a flipside here is that cases where no destructors should be generated -- particularly globals -- would be needed to completely remove destructor calls. Even for ranges, the range can often include the destructor placement. So I've shifted frame-of-thought a little: accept a bunch of destructor churn, because destructors are needed and will be prevalent. The verbosity is a feature of the design to make desugaring apparent in IR, not a bug.

@jonmeow
Copy link
Contributor Author

jonmeow commented Jun 16, 2025

Depends on #5677, I split off a little where it made clear sense.

@geoffromer
Copy link
Contributor

It looks like #5677 has been merged, so could you rebase on trunk? My workflow for large PRs relies pretty heavily on the "viewed" button, which only seems to appear when displaying changes from all commits.

@jonmeow
Copy link
Contributor Author

jonmeow commented Jun 17, 2025

It looks like #5677 has been merged, so could you rebase on trunk? My workflow for large PRs relies pretty heavily on the "viewed" button, which only seems to appear when displaying changes from all commits.

Done.

@jonmeow
Copy link
Contributor Author

jonmeow commented Jun 18, 2025

ping?

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.

2 participants