[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

[Zachary Turner via lldb-commits]

On Tue, Sep 11, 2018 at 4:28 AM Aleksandr Urakov via Phabricator <
reviews at 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20180911/ecd863d4/attachment-0001.html>