[Lldb-commits] [PATCH] D63622: [Target] Hoist LanguageRuntime::GetDeclVendor

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.


================
Comment at: source/API/SBTarget.cpp:1854-1859
+          if (vendor->FindDecls(const_typename, /*append*/ true,
+                                /*max_matches*/ 1, decls) > 0) {
+            if (CompilerType type =
+                    ClangASTContext::GetTypeForDecl(decls.front()))
               return LLDB_RECORD_RESULT(SBType(type));
           }
----------------
xiaobai wrote:
> jingham wrote:
> > xiaobai wrote:
> > > jingham wrote:
> > > > As soon as you start iterating over all language runtimes, I don't think you can use ClangASTContext::GetTypeForDecl, can you?  Not all language runtimes will be backed by a Clang AST.
> > > In principle, this is wrong. But FindDecl's deals with clang NamedDecl's, so this still makes sense right now. In the future we will need to make DeclVendor more TypeSystem/compiler agnostic.
> > If that's true, then you should signal the restriction by limiting the iteration over language runtimes to C-family ones.  Like:
> > 
> >      // DeclVendor only works for C Family languages at present.
> >      if (!Language::LanguageIsCFamily(runtime->GetLanguageType())
> >             continue;
> > 
> > Or maybe we need another "LanguageIsBackedByClangAST" check?  That would be more accurate...
> I could try to limit things based on language, but that makes it seem unlikely to get cleaned up in the future if DeclVendor is generalized beyond just using clang.
> 
> Another idea could be to introduce a method `DeclVendor::GetTypes`, returning a `std::vector<CompilerType>` which will do the job that is being done here. That will remove clang from the equation entirely. Of course, because of the way DeclVendor works now, that method will end up using clang internally, but then it will be easier to clean up when support for non-clang-based languages are added.
The second idea is clearly what's going to have to get done to generalize this to other TypeSystems.  I was thinking of limiting the language more as a signpost that this is not a truly general solution, just to decouple this patch from the work to generalize the DeclVendor interface.  On second thought, a FIXME comment to that effect is probably a more explicit way to do this.

Of course, if you want to fix how the DeclVendors work to make them handle other TypeSystems as part of that patch, that would be double plus good.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63622/new/

https://reviews.llvm.org/D63622