Skip to content

Unify logging of errors during position conversions #1161

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

Merged

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Apr 4, 2024

Instead of logging errors in position translation ad-hoc at the caller’s side (and ofter forgetting to do so), log these errors in LineTable. To be able to debug where the position conversion error is coming from, also log the file name and line number of the caller.

@ahoppen ahoppen requested a review from bnbarham April 4, 2024 17:43
@ahoppen ahoppen requested a review from benlangmuir as a code owner April 4, 2024 17:43
@ahoppen ahoppen force-pushed the unify-position-conversion-error-logging branch from 27d4c2c to a25b1d2 Compare April 4, 2024 18:04
@ahoppen
Copy link
Member Author

ahoppen commented Apr 4, 2024

@swift-ci Please test

@ahoppen ahoppen force-pushed the unify-position-conversion-error-logging branch from a25b1d2 to a499fa7 Compare April 4, 2024 22:37
@ahoppen
Copy link
Member Author

ahoppen commented Apr 4, 2024

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Apr 4, 2024

@swift-ci Please test Windows

Copy link
Contributor

@bnbarham bnbarham left a comment

Choose a reason for hiding this comment

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

Much cleaner 👍

// Line out of range.
logger.fault(
"""
Unable to get string index for \(line):\(utf16Column) because line is out of range \
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably obvious enough from the caller, but could help to add whether it's utf8 vs utf16 in all these messages. It's not super necessary for the line case, so maybe just add utf<n> in front of column for the column cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea 👍🏽

Instead of logging errors in position translation ad-hoc at the caller’s side (and ofter forgetting to do so), log these errors in `LineTable`. To be able to debug where the position conversion error is coming from, also log the file name and line number of the caller.

rdar://125545620
@ahoppen ahoppen force-pushed the unify-position-conversion-error-logging branch from a499fa7 to d62c4ce Compare April 5, 2024 13:57
@ahoppen
Copy link
Member Author

ahoppen commented Apr 5, 2024

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Apr 5, 2024

@swift-ci Please test Windows

@ahoppen ahoppen merged commit 0de726d into swiftlang:main Apr 5, 2024
1 check passed
@ahoppen ahoppen deleted the unify-position-conversion-error-logging branch April 5, 2024 21:02
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.

2 participants