lassefolger added a comment. In D114627#3306300 <https://reviews.llvm.org/D114627#3306300>, @clayborg wrote:
> Sorry I am late to making comments on this patch. > > This patch wants to add "llvm::StringRef scope" to the FindTypes calls, but > this is what "const CompilerDeclContext &parent_decl_ctx" is actually for. If > "parent_decl_ctx" is set, it is a declaration context that each type must > exist in. So the CompilerDeclContext might be something like "namespace foo" > and it might have a parent decl context of "namespace bar". This is currently > how filtering types that exist in a decl context works. > > The "scope" seems redundant in light of this no? Also, a string based "scope" > isn't accurate enough to correctly identify the exact scoping of a type as > "a:🅱️:some_type" might be "namespace a -> class b -> some_type" or "namespace > a -> namespace b -> some_type". > > The one difficult thing about this is currently the "parent_decl_ctx" that is > a parameter only works if the decl context comes from the current module. > Each module creates types using its own TypeSystem subclass. Those types are > vended as CompilerType objects which contain the TypeSystem pointer and a > void * for the type. This allows each TypeSystem to create types. For clang > compiled code we use lldb_private::TypeSystemClang and we create types as > types in a clang::ASTContext. We can also specify declaration contexts by > using CompilerDeclContext objects. Each CompilerDeclContext object has a > "TypeSystem *" and a "void *" inside of it. Each expression makes its own > TypeSystem as well, and one for each expression and one in the target. It > should be possible to fix the issue of needing the compiler decl context to > come from the current module by having the code that compares the decl > context make by using: > > ConstString CompilerDeclContext::GetScopeQualifiedName() const; > > Or we can modify the CompilerDeclContext class to get the current name: > > ConstString CompilerDeclContext::GetName() const; > > And then also add an API to return an enumeration for the kind of the > declaration context (Namespace, Class, Struct, Union, etc). The idea is that the scope is a hint which implementations can use to improve the performance. However, I see that this redundancy might be confusing. I have not looked to much into the ComplierDeclContext. I'll try to take another look later this week. FWIW there has been a discussion between werat@ and teemperor@ about this on Discord about this topic in general. The results was to use a string rather than a CompilerDeclContext. Although the discussion didn't go into detail why this is: https://discord.com/channels/636084430946959380/636732809708306432/849218944581632001 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114627/new/ https://reviews.llvm.org/D114627 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
