-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Bridging: Bridge some basic classes like swift::SourceLoc
directly
#82665
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
Conversation
@swift-ci please smoke test |
@swift-ci please smoke test |
@swift-ci please test Windows |
1 similar comment
@swift-ci please test Windows |
Storing a `llvm::SMLoc` is a superfluous indirection, and getting rid of it enables us to unconditionally import `SourceLoc` into Swift.
@swift-ci please test Windows |
StringRef str() const { | ||
return StringRef(Start.Value.getPointer(), ByteLength); | ||
} | ||
StringRef str() const { return StringRef(Start.Pointer, ByteLength); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What an evil method 😬 All the usages should use SourceManager::extractText()
instead. (Not in this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small comment. Otherwise, Looks good to me.
l = l.advanced(by: pound.trimmedLength.utf8Length) | ||
l = l.advanced(by: pound.trailingTriviaLength.utf8Length) | ||
l = l.advanced(by: node.openingQuote.leadingTriviaLength.utf8Length) | ||
l = l.advanced(by: Int32(pound.trimmedLength.utf8Length)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CInt
is more robust. But I think it's reasonable to change the C++ side to accept size_t
instead of int
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of places where we advance by a negative offset. I did try switching to intptr_t
, but that required some additional static casts that I chose to reserved for a subsequent PR, in calls that rely on unsigned
-> int
wrapping, as in loc.getAdvancedLoc(-presumedLoc.getColumn() + 1)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also use ssize_t
which is both signed and imports as Int
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried intptr_t
because
typedef intptr_t SwiftInt;
typedef uintptr_t SwiftUInt;
I’m tempted to move these to Basic/SwiftBridging.h
and use SwiftInt
for consistency.
@swift-ci please smoke test |
Thank you, Rintaro! |
@swift-ci please smoke test Windows |
No description provided.