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 > > > >
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits