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.

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

[cxx-interop] Import more C++ source locations into Swift #77204

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Xazax-hun
Copy link
Contributor

Occasionally, when the Swift compiler emits a diagnostic for a construct that was imported from C++ we get a diagnostic with unknown location. This is a bad user experience. It is particularly bad with the borrow-checker related diagnostics. This patch extends the source location importing to declarations in ClangImporter. There are some invariants enforced by the Swift compile, e.g., a source range is comprised of two valid source locations or two invalid ones. As a result, this patch adds approximate source locations to some separators like braces or parens that are not maintained by Clang. Having slightly incorrect ranges in this case is better than emitting unknown source locations.

Occasionally, when the Swift compiler emits a diagnostic for a construct
that was imported from C++ we get a diagnostic with unknown location.
This is a bad user experience. It is particularly bad with the
borrow-checker related diagnostics. This patch extends the source
location importing to declarations in ClangImporter. There are some
invariants enforced by the Swift compile, e.g., a source range is
comprised of two valid source locations or two invalid ones. As a
result, this patch adds approximate source locations to some separators
like braces or parens that are not maintained by Clang. Having slightly
incorrect ranges in this case is better than emitting unknown source
locations.
@Xazax-hun Xazax-hun added the c++ interop Feature: Interoperability with C++ label Oct 24, 2024
@Xazax-hun
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@egorzhdan egorzhdan left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor question.

else {
range = SourceRange(D->getNameLoc(), D->getNameLoc());
}
// Invariant: range.isValid() == D->getNameLoc().isValid()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be an assertion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants