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

Revert "Avoid copying IR nodes when no change is performed in passes" #11018

Closed
wants to merge 1 commit into from
Closed

Revert "Avoid copying IR nodes when no change is performed in passes" #11018

wants to merge 1 commit into from

Conversation

hubertp
Copy link
Collaborator

@hubertp hubertp commented Sep 9, 2024 •
edited
Loading

Pull Request Description

Partial revert of commit a65383a. Workaround for #10985.

This change renders #11015 obsolete. At least temporarily.

@hubertp hubertp added the CI: No changelog needed Do not require a changelog entry for this PR. label Sep 9, 2024
JaroslavTulach
JaroslavTulach reviewed Sep 10, 2024
View reviewed changes
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Partial revert of commit a65383a.

What does it mean "partial revert"?

This change renders #11015 obsolete. At least temporarily.

Issue #11015 is a temporary removal of the check targeted only for release/2024.4 branch.

@hubertp
Copy link
Collaborator Author

hubertp commented Sep 10, 2024

Partial revert of commit a65383a.

What does it mean "partial revert"?

"partial revert" is well... partial :) If you look up the original commit there are some additional changes related to reducing pattern matching which also improved performance but were otherwise equivalent in semantics. copying changes are the controversial/dangerous bit. Besides, there have been changes on top of a65383a so it is no longer possible to make a clean revert.

This change renders #11015 obsolete. At least temporarily.

Issue #11015 is a temporary removal of the check targeted only for release/2024.4 branch.

I'm still not convinced that a removal of the check does not keep some other bugs lurking around. Hence my preference for this PR rather than #11015.

@enso-bot enso-bot bot mentioned this pull request Sep 11, 2024
Closed
@hubertp
Copy link
Collaborator Author

hubertp commented Sep 17, 2024

The assert was disabled. I still feel like I'd like to get it back but no point in keeping the PR up for now.

@hubertp hubertp closed this Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JaroslavTulach JaroslavTulach JaroslavTulach left review comments

@4e6 4e6 Awaiting requested review from 4e6 4e6 is a code owner

@Akirathan Akirathan Awaiting requested review from Akirathan Akirathan is a code owner

Assignees
No one assigned
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hubertp @JaroslavTulach