xiaobai marked an inline comment as done. xiaobai 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)); } ---------------- 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? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63622/new/ https://reviews.llvm.org/D63622 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits