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

[Aleksandr Urakov via lldb-commits]

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 <zturner at google.com> wrote:

>
>
> 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
>>
>>
>>
>>

-- 
Aleksandr Urakov
Software Developer
JetBrains
http://www.jetbrains.com
The Drive to Develop
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20180911/5c5c00a5/attachment.html>