[Lldb-commits] [PATCH] D114627: [lldb] add new overload for SymbolFile::FindTypes that accepts a scope

[Lasse Folger via Phabricator via lldb-commits]

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