[Symbol] Remove swift from CompilerType#1903
[Symbol] Remove swift from CompilerType#1903bulbazord wants to merge 1 commit intoapple:upstream-with-swiftfrom
Conversation
|
@swift-ci please test |
|
Hmm, looks like the swift-lldb merge is ahead of the swift-llvm merge. LLDB is using |
adrian-prantl
left a comment
adrian-prantl
left a comment
There was a problem hiding this comment.
What's the motivation behind this patch?
Could you achieve the same by perhaps having a
class SwiftCompilerType : public CompilerType that does provide this constructor?
|
Can you post a link to the review were you removed clang from CompilerType? I'd like to read up on that to better understand what's going on. |
My motivation is more decoupling of swift from non-plugin libraries (lldbSymbol in this case). I think that we could probably have a SwiftCompilerType. If that's what we went with, I'd like for it to live in the SwiftExpressionParser plugin to avoid adding more swift-specific bits to non-plugin libraries.
Review was here: https://reviews.llvm.org/D66102 |
|
Thanks, I posted the same question on the llvm review. Whatever interface we decide on there should probably also be used here. That said, I don't think this necessarily needs to hold up this patch. |
|
Sounds good. Whatever happens on llvm.org, I'll mirror here for swift. I think landing this in the meantime should be okay (assuming I can get a build on swift-ci). |
|
|
||
| CompilerType return_ast_type(result_type.getPointer()); | ||
| CompilerType return_ast_type(SwiftASTContext::GetSwiftASTContext(&ast_context), | ||
| result_type.getPointer()); |
There was a problem hiding this comment.
This is dangerous, because it is not necessarily equivalent to the original code. Without having read the context, there is no guarantee that ast_context is equivalent to result_type->getASTContext(). There are many concurrent SwiftASTContexts floating around.
There was a problem hiding this comment.
I see. In that case, we should almost always be asking the swift::Type for its ASTContext when constructing a CompilerType then?
There was a problem hiding this comment.
If that's the case, then a wrapper or something would be a great idea. It would cut down on boilerplate significantly.
|
Yeah, I feel like the wrapper makes much more sense in the Swift case than in the Clang case. |
bulbazord
force-pushed
the
too-much-typing
branch
from
5894f26 to
177cd3b
Compare
|
@swift-ci please test |
adrian-prantl
left a comment
adrian-prantl
left a comment
There was a problem hiding this comment.
Ah, so the wrapper already was there?
|
I just added the wrapper actually. Not too bad to do it, even if a bit repetitive. |
bulbazord
force-pushed
the
too-much-typing
branch
from
177cd3b to
fe6948d
Compare
|
@swift-ci please test |
2 similar comments
|
@swift-ci please test |
|
@swift-ci please test |
bulbazord
force-pushed
the
too-much-typing
branch
from
fe6948d to
4e5185a
Compare
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
| lldb::StackFrameWP &stack_frame_wp, | ||
| swift::SourceFile *source_file, Status &error); | ||
|
|
||
| static CompilerType GetSwiftCompilerType(swift::Type swift_type); |
There was a problem hiding this comment.
isn't the "Swift" in the name redundant?
Should it be SwiftASTContext::GetCompilerType(...)?
There was a problem hiding this comment.
Yeah, it is somewhat redundant. I need to update this PR, I can rename it when I do that.
bulbazord
force-pushed
the
too-much-typing
branch
2 times, most recently
from
ed9b28f to
94cc320
Compare
|
@swift-ci please test |
2 similar comments
|
@swift-ci please test |
|
@swift-ci please test |
I removed clang from CompilerType, so removing swift seems like the natural progression
bulbazord
force-pushed
the
too-much-typing
branch
from
94cc320 to
5687e35
Compare
|
@swift-ci please test |
3 participants
I removed clang from CompilerType, so removing swift seems like the natural
progression.