[lldb] Push down instantiation of SwiftASTContext on tss-TypeRef #3688
Conversation
|
@swift-ci test |
There was a problem hiding this comment.
IIRC, I renamed this to target_holder because in the end we wanted to read the Target* from it. If that's true, maybe this should be a std::function<Target *()> target_provider?
There was a problem hiding this comment.
I agree, that sounds better.
There was a problem hiding this comment.
There's one situation where we need the ASTContext, so I think keeping it returning a SwiftASTContext is probably better.
} Impl(swift_ast_context ? swift_ast_context->GetASTContext() : nullptr);
There was a problem hiding this comment.
That's in GetClangTypeNode(CompilerType clang_type, swift::Demangle::Demangler &dem, SwiftASTContext *swift_ast_context) which doesn't call the argument target_holder. Maybe those two use-cases can be treated separately?
There was a problem hiding this comment.
Uh.. it's unfortunate that we need this there unconditionally. I guess the next task will be to see if we can factor out AST->getSwiftName() to not need a full SwiftASTContext initialization.
There was a problem hiding this comment.
I think it's better to keep it like this for now, as we're threading to both places from the same intermediary functions, and I think it'd be more confusing to treat them as separate (for now). If we factor out AST->getSwiftName() then we'll probably have to separate the cases.
There was a problem hiding this comment.
Is either of GetSwiftASTContextLazy() or GetLazySwiftASTContext() a more descriptive name?
There was a problem hiding this comment.
Yes, it looks like we really just want a TargetWP here.
There was a problem hiding this comment.
If we do go with the "lazy" naming scheme, we could call swift_ast_context_provider lazy_swift_ast_context
adrian-prantl
left a comment
adrian-prantl
left a comment
There was a problem hiding this comment.
I have a few naming suggestions inside, but otherwise this looks good!
augusto2112
force-pushed
the
get-swift-ast-context-provider
branch
from
18830ba to
8f891eb
Compare
|
@swift-ci test |
|
@swift-ci test Linux |
2 participants
No description provided.