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.  <br><br>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 <br><div class="gmail_quote"><div dir="ltr">On Tue, Sep 11, 2018 at 8:43 AM Aleksandr Urakov <<a href="mailto:aleksandr.urakov@jetbrains.com">aleksandr.urakov@jetbrains.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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.<div><br></div><div>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.</div><div><br></div><div>What do you think about it?</div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Sep 11, 2018 at 5:41 PM Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><br></div><div><br><div class="gmail_quote"><div dir="ltr">On Tue, Sep 11, 2018 at 4:28 AM Aleksandr Urakov via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">aleksandr.urakov added inline comments.<br>
<br>
<br>
================<br>
Comment at: lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:289<br>
+  return GetClassOrFunctionParent(*lexical_parent);<br>
+}<br>
+<br>
----------------<br>
zturner wrote:<br>
> 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:<br>
> <br>
> <br>
> ```<br>
> uint32_t ParentId = 0;<br>
> switch (raw.getSymTag()) {<br>
> case PDB_SymType::Array:<br>
> case PDB_SymType::BaseClass:<br>
> case PDB_SymType::BuiltinType:<br>
> case PDB_SymType::Enum:<br>
> case PDB_SymType::FunctionArg:<br>
> case PDB_SymType::FunctionType:<br>
> case PDB_SymType::PointerType:<br>
> case PDB_SymType::UDTType:<br>
> case PDB_SymType::VTable:<br>
> case PDB_SymType::VTableShape:<br>
>   assert(false && "This is a type not a symbol!");<br>
>   return nullptr;<br>
> default:<br>
>   break;<br>
> }<br>
> ```<br>
> <br>
> 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:<br>
> <br>
> template<typename T><br>
> static std::unique_ptr<llvm::pdb::PDBSymbol><br>
> classOrLexicalParent(const T& t, IPDBSession &Session) {<br>
>   uint32_t parent = t.getClassParentId();<br>
>   if (parent == 0)<br>
>     parent = t.getLexicalParentId();<br>
>   return Session.getSymbolById(parent);<br>
> }<br>
> <br>
> ```<br>
> if (auto &F = dyn_cast<PDBSymbolFunc>(symbol)) {<br>
>   return classOrLexicalParent(F);<br>
> } else if (auto &D = dyn_cast<PDBSymbolData>(symbol)) {<br>
>   return classOrLexicalParent(F);<br>
> } else if (auto &P = dyn_cast<PDBSymbolPublic>(symbol)) {<br>
>   return P.getLexicalParent();<br>
> }<br>
> <br>
> ```<br>
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.<br>
<br>
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.<br>
</blockquote><div dir="auto">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</div><div dir="auto"><br></div><div dir="auto"><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D51162" rel="noreferrer" target="_blank">https://reviews.llvm.org/D51162</a><br>
<br>
<br>
<br>
</blockquote></div></div>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="m_-5429343210760690326gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div>Aleksandr Urakov</div><div><span>Software Developer</span></div><div><span>JetBrains</span></div><div><span><a href="http://www.jetbrains.com" target="_blank">http://www.jetbrains.com</a></span></div><div><span>The Drive to Develop</span></div></div></div>
</blockquote></div>