Skip to content

Conversation

jamieQ
Copy link
Contributor

@jamieQ jamieQ commented Aug 22, 2025

A number of existing comments in the DI pass alluded to box to stack promotion and assign lowering, both of which seem like they are vestiges of a fairly distant past. Update comments where appropriate. Also DRY a utility in a local function.

// removing it.

// Helper to remove the instruction from our data structures.
auto eraseUseInst = [&] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i couldn't resist hoisting this since it seemed like it was just copy-pasted over time... out of curiosity, why are the instructions removed from the Use/NonLoadUses structures in these cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

🤷

@jamieQ
Copy link
Contributor Author

jamieQ commented Aug 22, 2025

@swift-ci please smoke test

@jamieQ jamieQ marked this pull request as ready for review August 22, 2025 01:58
@jamieQ jamieQ requested a review from eeckstein as a code owner August 22, 2025 01:58
Copy link
Contributor

@eeckstein eeckstein left a comment

Choose a reason for hiding this comment

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

lgtm

// removing it.

// Helper to remove the instruction from our data structures.
auto eraseUseInst = [&] {
Copy link
Contributor

Choose a reason for hiding this comment

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

🤷

@jamieQ
Copy link
Contributor Author

jamieQ commented Aug 22, 2025

@swift-ci please smoke test linux platform

@jamieQ
Copy link
Contributor Author

jamieQ commented Aug 22, 2025

@swift-ci please smoke test windows platform

2 similar comments
@jamieQ
Copy link
Contributor Author

jamieQ commented Aug 22, 2025

@swift-ci please smoke test windows platform

@jamieQ
Copy link
Contributor Author

jamieQ commented Aug 23, 2025

@swift-ci please smoke test windows platform

@jamieQ
Copy link
Contributor Author

jamieQ commented Aug 24, 2025

@eeckstein thanks the the review! could you merge this when you have the chance please?

also, cc @janbaig – i suspect there may be potential merge conflicts between this and #83886 just FYI depending on which makes it in first

@janbaig
Copy link
Contributor

janbaig commented Aug 24, 2025

@jamieQ Got it, thanks for the heads up!

A number of existing comments in the DI pass alluded to box to stack
promotion and assign lowering, both of which seem like they are vestiges
of a fairly distant past. Update comments where appropriate. Also DRY a
utility in a local function.
@jamieQ jamieQ force-pushed the remove-old-di-comments branch from 13235d8 to bc12429 Compare September 7, 2025 19:58
@jamieQ
Copy link
Contributor Author

jamieQ commented Sep 7, 2025

@swift-ci please smoke test

@jamieQ
Copy link
Contributor Author

jamieQ commented Sep 8, 2025

@eeckstein would you merge this if it still seems okay to you please (i made some minor edits to the original comments and resolved a merge conflict)

@eeckstein eeckstein merged commit 1a8d6a8 into swiftlang:main Sep 22, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants