Skip to content

Cleaning up anchor finding in interpolated strings #41723

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
Merged
merged 1 commit into from
Mar 9, 2022
Merged

Cleaning up anchor finding in interpolated strings #41723

merged 1 commit into from
Mar 9, 2022

Conversation

etcwilde
Copy link
Member

@etcwilde etcwilde commented Mar 8, 2022

The arglist stuff didn't exist when I originally wrote this.
With that, we can knock down the pyramid of dyn-casts and make it look nicer.

@etcwilde etcwilde requested a review from hamishknight March 8, 2022 08:24
@etcwilde
Copy link
Member Author

etcwilde commented Mar 8, 2022

@swift-ci please test

@etcwilde
Copy link
Member Author

etcwilde commented Mar 8, 2022

Hmm, this isn't doing as much as I was hoping for. There used to be a tower of dyn-casts, but it doesn't look so bad.
The asserts could stay, or could not. If anything needs to go, it's probably just the comment really.
Anyway, I should probably go to bed instead of trying to make decisions at midnight.

hamishknight
hamishknight approved these changes Mar 8, 2022
View reviewed changes
Copy link
Contributor

@hamishknight hamishknight left a comment

Choose a reason for hiding this comment

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

Makes sense to me if the assertions are no longer needed, I can't say I'm too familiar with this code tho

@etcwilde
Copy link
Member Author

etcwilde commented Mar 8, 2022

@swift-ci please test macOS

@etcwilde
Copy link
Member Author

etcwilde commented Mar 8, 2022

Yeah, I don't think that most of them are necessary anymore.
Most of them were meant to trigger if my assumptions were off about how interpolated strings worked, since that would result in the await insertion fix-it being emitted into the wrong location. I haven't heard anything about crashes here or issues with incorrect fix-its in about a year now, so I think it's safe to trim them back. Especially given that the argument list mechanism is more generalized.
We probably could keep the check that parent is a nullptr though because that would indicate that isEffectAnchor is not working correctly.

@etcwilde
Cleaning up anchor finding in interpolated strings
f789974 
The arglist didn't exist when I originally wrote this. Most of the
assertions were there to check my assumptions about the dyn-casts that
were being performed originally. In a non-asserts build, the dyn-casts
would not crash, but would emit the `await` insertion fix-it into the
wrong location. In an asserts build, it would crash instead.
I haven't heard about crashes, or misplaced fix-its from this for
roughly a year now, so I think it's safe to clean it up.

I'm keeping the `parent == nullptr` check since that would indicate an
issue in the implementation of `isEffectAnchor`.
@etcwilde etcwilde force-pushed the ewilde/garden-anchors branch from 371c55f to f789974 Compare March 8, 2022 17:13
@etcwilde
Copy link
Member Author

etcwilde commented Mar 8, 2022

@swift-ci please test

@etcwilde etcwilde merged commit cf05e61 into swiftlang:main Mar 9, 2022
@etcwilde etcwilde deleted the ewilde/garden-anchors branch March 11, 2022 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hamishknight hamishknight hamishknight approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@etcwilde @hamishknight