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

[Aleksandr Urakov via lldb-commits]

Agreed. I'll implement it tomorrow, thanks!

On Tue, Sep 11, 2018 at 7:24 PM Zachary Turner <zturner at google.com> wrote:

> A visitor would work, but unless we need this frequently it might be
> overkill. If we do need it frequently then it would be very helpful though.
>
> For now since we just have this one use case I think a switch statement on
> the tag is the simplest. You can group all same cases together and use the
> raw symbol to call one method or the other. At least then the reader can
> understand the hierarchy
> On Tue, Sep 11, 2018 at 8:43 AM Aleksandr Urakov <
> aleksandr.urakov at jetbrains.com> wrote:
>
>> 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
>>
>

-- 
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/00688a9b/attachment.html>