Yes, you are right, we can just to consider some cases important for us in the function, but I thought to solve the problem in a more general way.
I think that the key problem is that we lose some type info when we get a `PDBSymbol`. So we need to dyn_cast the symbol multiple times or to analyse the tag. To avoid this we can reproduce a huge hierarchy of interfaces (e.g. IPDBSymbolHasLexicalParent etc.) and then cast the symbol to the required interface and call the required function if the cast was successful. But I think that there is a more simple solution, we can introduce a visitor for PDBSymbol. It could help us to solve such problems in the future in a type-safe way without any casts. What do you think about it? On Tue, Sep 11, 2018 at 5:41 PM Zachary Turner <ztur...@google.com> wrote: > > > On Tue, Sep 11, 2018 at 4:28 AM Aleksandr Urakov via Phabricator < > revi...@reviews.llvm.org> wrote: > >> aleksandr.urakov added inline comments. >> >> >> ================ >> Comment at: lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:289 >> + return GetClassOrFunctionParent(*lexical_parent); >> +} >> + >> ---------------- >> zturner wrote: >> > However, this is only for symbols (not types). So we can exclude >> everything above Compiland. In fact, we should `assert` that we don't get >> anything from above Compiland. So I would write this as: >> > >> > >> > ``` >> > uint32_t ParentId = 0; >> > switch (raw.getSymTag()) { >> > case PDB_SymType::Array: >> > case PDB_SymType::BaseClass: >> > case PDB_SymType::BuiltinType: >> > case PDB_SymType::Enum: >> > case PDB_SymType::FunctionArg: >> > case PDB_SymType::FunctionType: >> > case PDB_SymType::PointerType: >> > case PDB_SymType::UDTType: >> > case PDB_SymType::VTable: >> > case PDB_SymType::VTableShape: >> > assert(false && "This is a type not a symbol!"); >> > return nullptr; >> > default: >> > break; >> > } >> > ``` >> > >> > And for the rest it's quite simple because you probably really only >> care about functions, public symbols, and data. In which case you can >> continue: >> > >> > template<typename T> >> > static std::unique_ptr<llvm::pdb::PDBSymbol> >> > classOrLexicalParent(const T& t, IPDBSession &Session) { >> > uint32_t parent = t.getClassParentId(); >> > if (parent == 0) >> > parent = t.getLexicalParentId(); >> > return Session.getSymbolById(parent); >> > } >> > >> > ``` >> > if (auto &F = dyn_cast<PDBSymbolFunc>(symbol)) { >> > return classOrLexicalParent(F); >> > } else if (auto &D = dyn_cast<PDBSymbolData>(symbol)) { >> > return classOrLexicalParent(F); >> > } else if (auto &P = dyn_cast<PDBSymbolPublic>(symbol)) { >> > return P.getLexicalParent(); >> > } >> > >> > ``` >> The problem is that this function is used also for type symbols. When >> creating a type, we use `GetDeclContextContainingSymbol` to find a correct >> context for the type declaration, and call `GetClassOrFunctionParent` >> there. So we can find an enclosing class for an inner class for example. >> >> The main reason why I have used the low-level interface here is to avoid >> extra dynamic casts. But what you are saying about (if I understand >> correctly) is the type safety, and may be it's worth several dynamic casts. >> Do you mind if I'll commit the fix for the test and the warning and then >> will try to figure out (considering the table you have sent) how to make >> this function more type safe? I think that it requires some more time to >> find the solution. >> > It’s not just about type safety but also readability. Handling each case > makes the expectations more clear so someone reading the code will have a > better idea what it’s doing. Using the raw interface might still be fine > if the cases are enumerated and getClassParent / getLexicalParent are only > called when it makes sense > > > >> >> Repository: >> rL LLVM >> >> https://reviews.llvm.org/D51162 >> >> >> >> -- Aleksandr Urakov Software Developer JetBrains http://www.jetbrains.com The Drive to Develop
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits