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

PrettyPrinter reports wrong line LineNumbersTests #883

Merged
merged 1 commit into from
Dec 4, 2024
Merged

PrettyPrinter reports wrong line LineNumbersTests #883

merged 1 commit into from
Dec 4, 2024

Conversation

bkolb
Copy link
Contributor

@bkolb bkolb commented Nov 23, 2024 •
edited
Loading

This is to reproduce issue #882.

I am not sure if the fix is done in the right place. With my change every string is checked for new lines, which might be not necessary and comes at a slight cost. Maybe there is another, better place to fix this.

@bkolb bkolb force-pushed the bug/882-wrongLineNumberWithComments branch 2 times, most recently from 0852ed5 to 9b47a88 Compare November 23, 2024 22:16
ahoppen
ahoppen reviewed Dec 3, 2024
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @bkolb! The fix looks good to me, I just have one small comment.

Sources/SwiftFormat/PrettyPrint/PrettyPrintBuffer.swift Outdated Show resolved Hide resolved
@bkolb bkolb force-pushed the bug/882-wrongLineNumberWithComments branch from 9b47a88 to 028ad94 Compare December 3, 2024 12:48
ahoppen
ahoppen approved these changes Dec 3, 2024
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@ahoppen ahoppen enabled auto-merge December 3, 2024 22:19
@ahoppen
Copy link
Member

ahoppen commented Dec 4, 2024

@bkolb Could you run swift-format on your changes? The Windows failures will be fixed by swiftlang/github-workflows#70.

auto-merge was automatically disabled December 4, 2024 12:30

Head branch was pushed to by a user without write access

@bkolb bkolb force-pushed the bug/882-wrongLineNumberWithComments branch from 028ad94 to 1af4c17 Compare December 4, 2024 12:30
@bkolb
Copy link
Contributor Author

bkolb commented Dec 4, 2024

@bkolb Could you run swift-format on your changes? The Windows failures will be fixed by swiftlang/github-workflows#70.

Done

allevato
allevato approved these changes Dec 4, 2024
View reviewed changes
Copy link
Member

@allevato allevato left a comment

Choose a reason for hiding this comment

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

Thanks for digging into this! Turns out the problem was nowhere near where I was guessing it might be.

@bkolb
Copy link
Contributor Author

bkolb commented Dec 4, 2024

No problem :-) Thx for your reviews.

I don't understand the build failure on windows. Can you give me a hint?

@ahoppen
Copy link
Member

ahoppen commented Dec 4, 2024

The Windows test failure is an infrastructure issue and will be fixed by swiftlang/github-workflows#70.

@award999
Copy link

award999 commented Dec 4, 2024

@bkolb can we try changing https://github.com/swiftlang/swift-format/blob/main/.github/workflows/pull_request.yml#L10C5-L10C83 to uses: award999/github-workflows/.github/workflows/swift_package_test.yml@wrap-source, if that works then can merge the workflow fix

@bkolb
PrettyPrinter reports wrong line LineNumbersTests
2cd032c 
The reason for the wrong line number were multiline
comments.
In to accomodate for this, we now check the string
while writing for new lines and increment the line
count accordingly.

Issue: swiftlang#882
@bkolb bkolb force-pushed the bug/882-wrongLineNumberWithComments branch from 1af4c17 to 2cd032c Compare December 4, 2024 18:04
@ahoppen ahoppen enabled auto-merge December 4, 2024 18:09
@ahoppen ahoppen merged commit 0c5afee into swiftlang:main Dec 4, 2024
19 checks passed
@ahoppen ahoppen mentioned this pull request Dec 10, 2024
Open
@ahoppen
Copy link
Member

ahoppen commented Dec 10, 2024

@bkolb This PR regressed swift-format’s performance by ~7% (#894). Could you check if there’s a way to avoid that performance regression?

@allevato
Copy link
Member

That split call is doing a lot of work that gets thrown away. We don't actually use the substrings themselves, just the number of them and the length of the last one, so we have allocations on the critical printing path that we don't need. Replacing that with something that just counts the number of \ns and the length of the final segment would probably yield a good improvement.

@bkolb
Copy link
Contributor Author

bkolb commented Dec 11, 2024

Sorry for the delay.
I can look at that hopefully tomorrow.
Do you have some performance test suite I can use to benchmark?

@ahoppen
Copy link
Member

ahoppen commented Dec 12, 2024

#894 contains instruction how to reproduce the measurement.

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

@ahoppen ahoppen ahoppen approved these changes

@allevato allevato allevato approved these changes

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

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

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

Successfully merging this pull request may close these issues.
4 participants
@bkolb @ahoppen @award999 @allevato