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:
> > > > 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.
> Okay, so then what I think I'm going to do is this:
> - Add a FIXME to this patch denoting that this is really will only work with
> clang at the moment. Unfortunate, but easily findable with a quick `git grep
> FIXME`.
> - Submit a patch that adds a method `DeclVendor::GetTypes` which will take on
> the burden of using clang. The FIXME will be moved there until I figure out
> how to generalize DeclVendor further.
>
> Sound good to you?
That sounds like a fine plan.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63622/new/
https://reviews.llvm.org/D63622
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
