-
Notifications
You must be signed in to change notification settings - Fork 339
[lldb] Build DeclContext vector from a Swift demangle tree #8465
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
[lldb] Build DeclContext vector from a Swift demangle tree #8465
Conversation
@swift-ci test |
Depends on #8464 |
af8fca4
to
8f84352
Compare
@swift-ci test |
@swift-ci test Windows |
static std::optional<std::pair<StringRef, StringRef>> | ||
GetNominal(swift::Demangle::Demangler &dem, swift::Demangle::NodePointer node) { | ||
bool | ||
TypeSystemSwiftTypeRef::IsBuiltinType(CompilerType type) { |
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.
Maybe call this IsSwiftBuiltinType?
alternatively — is this different from CmpilerType.IsIntegerType() || CmpilerType.IsFloatingPointType() || CmpilerType.IsVectorType()
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.
Is this even used in the patch?
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.
This function is unrelated, the diff is probably highlighting it because I changed code above and below it?
lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp
Outdated
Show resolved
Hide resolved
if (!first) | ||
return false; | ||
if (first->getKind() == Node::Kind::Module) { | ||
assert(first->hasText() && "Module node should have text!"); |
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.
this also needs to be a dynamic check.
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.
Also I see at least two other places in the same file that disect TypeAlias nodes, so factoring this out into a helper might be best.
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 should the helper do though? There aren't many assumptions you can make about the type's children, as both nodes could be many different things.
The first can be the module, another type, I believe also a function.
The second one can be an identifier, a private decl, or a local decl.
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 guess we could add a generic
[declcontext, type] = extractBinaryNode(node)
facility. If you don't think that's worth it, then not.
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 added two helpers for nodes that I thought made sense, but left the rest as is
return false; | ||
|
||
auto *first = node->getChild(0); | ||
auto *second = node->getChild(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.
same thing here about factoring this out. I think this will get a lot more readable if it starts with:
std::optional<std::pair<StringRef, StringRef>> module_ident = extractFunctionNode(node);
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.
Like, the other comment, there's not much we can assume about the children, so the helper couldn't do much besides verifying the number of children and just returning them.
lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp
Outdated
Show resolved
Hide resolved
/// Return a pair of module name and type name, given a mangled name. | ||
static std::optional<std::pair<StringRef, StringRef>> | ||
GetNominal(llvm::StringRef mangled_name, swift::Demangle::Demangler &dem) { | ||
static std::optional<std::vector<CompilerContext>> |
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.
Doxygen comment would be nice.
struct S2 { | ||
func s2() { | ||
func s2_2() { | ||
struct S3 { |
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.
does this get noticeable different if, e.g., S3 were a class?
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.
Ok I changed this to use classes, enums and structs, plus a private discriminator
Replace the current GetNominal function, which would only handle finding a module + a type with a more complete BuildDeclContext function, which will recursively build a DeclContext, taking into account nested and private types. rdar://125044318
8f84352
to
08e70a2
Compare
@swift-ci test |
@swift-ci test Windows |
Replace the current GetNominal function, which would only handle finding
a module + a type with a more complete BuildDeclContext function, which
will recursively build a DeclContext, taking into account nested and
private types.
rdar://125044318