Skip to content

Conversation

@owenv
Copy link
Contributor

@owenv owenv commented Apr 18, 2020

  • Always use the line number in the actual source file when extracting excerpts and adding highlights/fix-its (SourceManager::getLineNumber and SourceManager::getLineAndCol return the real and virtual line numbers respectively)
  • Always use the display name when printing excerpt titles
  • Don't print surrounding lines when an annotated line is in a virtual file

@owenv
Copy link
Contributor Author

owenv commented Apr 18, 2020

@swift-ci smoke test

@owenv owenv force-pushed the pretty-printed-diags-source-location-fixes branch from 4a17a9f to aa43637 Compare April 20, 2020 17:10
@owenv
Copy link
Contributor Author

owenv commented Apr 20, 2020

The second commit is an additional test case I added to make sure the new style works when swift emits a diagnostic in a clang-owned buffer, which also interacts with virtual files

@owenv
Copy link
Contributor Author

owenv commented Apr 20, 2020

@swift-ci please smoke test

@owenv owenv requested a review from xedin April 20, 2020 17:12
@owenv
Copy link
Contributor Author

owenv commented Apr 20, 2020

@swift-ci please smoke test macOS

@owenv owenv force-pushed the pretty-printed-diags-source-location-fixes branch from aa43637 to 0fd1821 Compare April 24, 2020 03:16
owenv added 2 commits May 12, 2020 18:40
…sourceLocation is used

- Always use the line number in the actual source file when extracting excerpts and adding highlights/fix-its
- Always use the display name when printing excerpt titles
- Don't print surrounding lines when an annotated line is in a virtual file

This reverts commit f919e04.
@owenv owenv force-pushed the pretty-printed-diags-source-location-fixes branch from 0fd1821 to 96696c8 Compare May 13, 2020 01:45
@owenv
Copy link
Contributor Author

owenv commented May 13, 2020

@swift-ci smoke test

@owenv
Copy link
Contributor Author

owenv commented May 19, 2020

@rintaro I saw you posted about some of the issues working with real and virtual source locations a few years ago here: https://forums.swift.org/t/filename-line-and-column-for-sourceloc/5463

Do you mind looking over these changes, I think I'm fixing a similar kind of problem? I don't know if adding an isLocInVirtualFile method to the source manager is a good idea, but I haven't thought of a good alternative yet.

Also, are you still interested in renaming some of these methods? If so, I might try to rename at least SourceManager::getLineNumber and SourceManager::getLineAndCol in a follow-up PR.

@owenv owenv requested a review from rintaro May 19, 2020 20:38
Copy link
Member

@rintaro rintaro left a comment

Choose a reason for hiding this comment

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

The change looks good. As for isLocInVirtualFile() I cannot think of any better option.

Also, are you still interested in renaming some of these methods? If so, I might try to rename at least SourceManager::getLineNumber and SourceManager::getLineAndCol in a follow-up PR.

Yes, the current method names are very confusing and error prone. Thank you for working on this!

@owenv
Copy link
Contributor Author

owenv commented May 20, 2020

@swift-ci smoke test and merge

@swift-ci swift-ci merged commit 0ab9fd7 into swiftlang:master May 20, 2020
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