We could use clang::Decl* etc. in these classes but that would defeat the whole concept that they are supposed to implement. CompilerType, CompilerDecl, etc.. are all classes that represent their respective equivalent in a language plugin. For Clang that is indeed clang::Type* and clang::Decl*, but Swift is storing swift::Type* and swift::Decl* in these void* pointers (and passes a SwiftASTContext* instead of a ClangASTContext* as a m_type_system). So if we change that to clang::Decl* then the all other language plugins like Swift are effectively broken. It would also mean that generic LLDB code using these classes would have a hard dependency on Clang.
And there shouldn’t be any place where we unconditionally cast this to clang::Decl* as this would directly crash swift-lldb. The m_type_system (which is either ClangASTContext or SwiftASTContext) can freely cast the void* to clang::Decl/swift::Decl as the checking happens through the virtual function call to the m_type_system interface (Swift types always have a SwiftASTContext as m_type_system, Clang types have a ClangASTContext as m_type_system). Having said that, I’m not saying that the void* pointers in these classes are the best solution we could have here (there is a reason why I keep removing all these void* pointer conversions). If you see a way to get rid of them, then be my guest. > On 3. Jan 2020, at 01:52, Shafik Yaghmour <syaghm...@apple.com> wrote: > > To further clarify all the member functions of CompilerDecl e.g.: > > GetMangledName(…) > GetDeclContext(…) > > End up passing m_opaque_decl as an argument to a member function of > m_type_system and in these member functions they unconditionally cast the > argument to Decl* so why not make m_opaque_decl a Decl* > >> On Jan 2, 2020, at 4:20 PM, Shafik Yaghmour via lldb-commits >> <lldb-commits@lists.llvm.org> wrote: >> >> Apologies, m_opaque_decl member of CompilerDecl and operator < in >> CompilerDecl.h >> >>> On Jan 2, 2020, at 3:54 PM, Raphael “Teemperor” Isemann >>> <teempe...@gmail.com> wrote: >>> >>> Not sure if I can follow. What variable should just be a Decl* and what >>> operator<? >>> >>> (Also yeah that reinterpret_cast could also be a static_cast) >>> >>>> On Jan 3, 2020, at 12:38 AM, Shafik Yaghmour <syaghm...@apple.com> wrote: >>>> >>>> I am not crazy about the reinterpret_cast although if we look inside >>>> CompilerDecl impl we can see that basically it is always assuming it is a >>>> Decl* and just C-style casts it as such. So why not just make it a Decl* >>>> >>>> Also operator<(…) is super sketchy doing a < on void* >>>> >>>>> On Dec 28, 2019, at 6:52 AM, Raphael Isemann via lldb-commits >>>>> <lldb-commits@lists.llvm.org> wrote: >>>>> >>>>> >>>>> Author: Raphael Isemann >>>>> Date: 2019-12-28T15:20:19+01:00 >>>>> New Revision: 8612e92ed590e615f9f56e4fb86a1fdaf3a39e15 >>>>> >>>>> URL: >>>>> https://github.com/llvm/llvm-project/commit/8612e92ed590e615f9f56e4fb86a1fdaf3a39e15 >>>>> DIFF: >>>>> https://github.com/llvm/llvm-project/commit/8612e92ed590e615f9f56e4fb86a1fdaf3a39e15.diff >>>>> >>>>> LOG: [lldb][NFC] Remove GetASTContext call in ClangDeclVendor >>>>> >>>>> Instead of returning NamedDecls and then calling GetASTContext >>>>> to find back the ClangASTContext we used can just implement the >>>>> FindDecl variant that returns CompilerDecls (and implement the >>>>> other function by throwing away the ClangASTContext part of the >>>>> compiler decl). >>>>> >>>>> Added: >>>>> >>>>> >>>>> Modified: >>>>> lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.cpp >>>>> lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.h >>>>> lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp >>>>> lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp >>>>> lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.h >>>>> >>>>> Removed: >>>>> >>>>> >>>>> >>>>> ################################################################################ >>>>> diff --git >>>>> a/lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.cpp >>>>> b/lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.cpp >>>>> index c59722b7b4f8..0c5796650d45 100644 >>>>> --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.cpp >>>>> +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.cpp >>>>> @@ -15,16 +15,17 @@ using namespace lldb_private; >>>>> >>>>> uint32_t ClangDeclVendor::FindDecls(ConstString name, bool append, >>>>> uint32_t max_matches, >>>>> - std::vector<CompilerDecl> &decls) { >>>>> + std::vector<clang::NamedDecl *> >>>>> &decls) { >>>>> if (!append) >>>>> decls.clear(); >>>>> >>>>> - std::vector<clang::NamedDecl *> named_decls; >>>>> - uint32_t ret = FindDecls(name, /*append*/ false, max_matches, >>>>> named_decls); >>>>> - for (auto *named_decl : named_decls) { >>>>> - decls.push_back(CompilerDecl( >>>>> - ClangASTContext::GetASTContext(&named_decl->getASTContext()), >>>>> - named_decl)); >>>>> + std::vector<CompilerDecl> compiler_decls; >>>>> + uint32_t ret = FindDecls(name, /*append*/ false, max_matches, >>>>> compiler_decls); >>>>> + for (CompilerDecl compiler_decl : compiler_decls) { >>>>> + clang::Decl *d = >>>>> + reinterpret_cast<clang::Decl *>(compiler_decl.GetOpaqueDecl()); >>>>> + clang::NamedDecl *nd = llvm::cast<clang::NamedDecl>(d); >>>>> + decls.push_back(nd); >>>>> } >>>>> return ret; >>>>> } >>>>> >>>>> diff --git >>>>> a/lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.h >>>>> b/lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.h >>>>> index 85a10400a201..0c888de08841 100644 >>>>> --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.h >>>>> +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.h >>>>> @@ -21,12 +21,10 @@ class ClangDeclVendor : public DeclVendor { >>>>> >>>>> virtual ~ClangDeclVendor() {} >>>>> >>>>> - uint32_t FindDecls(ConstString name, bool append, uint32_t max_matches, >>>>> - std::vector<CompilerDecl> &decls) override; >>>>> + using DeclVendor::FindDecls; >>>>> >>>>> - virtual uint32_t FindDecls(ConstString name, bool append, >>>>> - uint32_t max_matches, >>>>> - std::vector<clang::NamedDecl *> &decls) = 0; >>>>> + uint32_t FindDecls(ConstString name, bool append, uint32_t max_matches, >>>>> + std::vector<clang::NamedDecl *> &decls); >>>>> >>>>> static bool classof(const DeclVendor *vendor) { >>>>> return vendor->GetKind() >= eClangDeclVendor && >>>>> >>>>> diff --git >>>>> a/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp >>>>> b/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp >>>>> index ff0905dda4ef..0696c669f2e2 100644 >>>>> --- >>>>> a/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp >>>>> +++ >>>>> b/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp >>>>> @@ -80,7 +80,7 @@ class ClangModulesDeclVendorImpl : public >>>>> ClangModulesDeclVendor { >>>>> Stream &error_stream) override; >>>>> >>>>> uint32_t FindDecls(ConstString name, bool append, uint32_t max_matches, >>>>> - std::vector<clang::NamedDecl *> &decls) override; >>>>> + std::vector<CompilerDecl> &decls) override; >>>>> >>>>> void ForEachMacro(const ModuleVector &modules, >>>>> std::function<bool(const std::string &)> handler) >>>>> override; >>>>> @@ -356,7 +356,7 @@ bool >>>>> ClangModulesDeclVendorImpl::AddModulesForCompileUnit( >>>>> uint32_t >>>>> ClangModulesDeclVendorImpl::FindDecls(ConstString name, bool append, >>>>> uint32_t max_matches, >>>>> - std::vector<clang::NamedDecl *> >>>>> &decls) { >>>>> + std::vector<CompilerDecl> &decls) { >>>>> if (!m_enabled) { >>>>> return 0; >>>>> } >>>>> @@ -382,7 +382,7 @@ ClangModulesDeclVendorImpl::FindDecls(ConstString >>>>> name, bool append, >>>>> if (num_matches >= max_matches) >>>>> return num_matches; >>>>> >>>>> - decls.push_back(named_decl); >>>>> + decls.push_back(CompilerDecl(m_ast_context.get(), named_decl)); >>>>> ++num_matches; >>>>> } >>>>> >>>>> >>>>> diff --git >>>>> a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp >>>>> >>>>> b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp >>>>> index 54f8397e1dad..29930c303b07 100644 >>>>> --- >>>>> a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp >>>>> +++ >>>>> b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp >>>>> @@ -537,10 +537,9 @@ bool >>>>> AppleObjCDeclVendor::FinishDecl(clang::ObjCInterfaceDecl *interface_decl) >>>>> { >>>>> return true; >>>>> } >>>>> >>>>> -uint32_t >>>>> -AppleObjCDeclVendor::FindDecls(ConstString name, bool append, >>>>> - uint32_t max_matches, >>>>> - std::vector<clang::NamedDecl *> &decls) { >>>>> +uint32_t AppleObjCDeclVendor::FindDecls(ConstString name, bool append, >>>>> + uint32_t max_matches, >>>>> + std::vector<CompilerDecl> >>>>> &decls) { >>>>> static unsigned int invocation_id = 0; >>>>> unsigned int current_id = invocation_id++; >>>>> >>>>> @@ -587,7 +586,7 @@ AppleObjCDeclVendor::FindDecls(ConstString name, bool >>>>> append, >>>>> current_id, result_iface_type.getAsString(), isa_value); >>>>> } >>>>> >>>>> - decls.push_back(result_iface_decl); >>>>> + decls.push_back(CompilerDecl(&m_ast_ctx, result_iface_decl)); >>>>> ret++; >>>>> break; >>>>> } else { >>>>> @@ -630,7 +629,7 @@ AppleObjCDeclVendor::FindDecls(ConstString name, bool >>>>> append, >>>>> new_iface_type.getAsString(), (uint64_t)isa); >>>>> } >>>>> >>>>> - decls.push_back(iface_decl); >>>>> + decls.push_back(CompilerDecl(&m_ast_ctx, iface_decl)); >>>>> ret++; >>>>> break; >>>>> } while (false); >>>>> >>>>> diff --git >>>>> a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.h >>>>> >>>>> b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.h >>>>> index 311113a27735..f49ca3540c2c 100644 >>>>> --- >>>>> a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.h >>>>> +++ >>>>> b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.h >>>>> @@ -28,7 +28,7 @@ class AppleObjCDeclVendor : public ClangDeclVendor { >>>>> } >>>>> >>>>> uint32_t FindDecls(ConstString name, bool append, uint32_t max_matches, >>>>> - std::vector<clang::NamedDecl *> &decls) override; >>>>> + std::vector<CompilerDecl> &decls) override; >>>>> >>>>> friend class AppleObjCExternalASTSource; >>>>> >>>>> >>>>> >>>>> >>>>> _______________________________________________ >>>>> lldb-commits mailing list >>>>> lldb-commits@lists.llvm.org >>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits >>>> >>> >> >> _______________________________________________ >> lldb-commits mailing list >> lldb-commits@lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits > _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
