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.


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
      • ... Zachary Turner via Phabricator via lldb-commits
  • [Ll... Aaron Smith via Phabricator via lldb-commits
  • [Ll... Aleksandr Urakov via Phabricator via lldb-commits
  • [Ll... Aleksandr Urakov via Phabricator via lldb-commits
  • [Ll... Stella Stamenova via Phabricator via lldb-commits
  • [Ll... Aleksandr Urakov via Phabricator via lldb-commits
  • [Ll... Ted Woodward via Phabricator via lldb-commits
  • [Ll... Zachary Turner via Phabricator via lldb-commits
  • [Ll... Aleksandr Urakov via Phabricator via lldb-commits
  • [Ll... Aleksandr Urakov via Phabricator via lldb-commits
  • [Ll... Aleksandr Urakov via Phabricator via lldb-commits
    • ... Zachary Turner via lldb-commits
      • ... Aleksandr Urakov via lldb-commits
        • ... Zachary Turner via lldb-commits
          • ... Aleksandr Urakov via lldb-commits
  • [Ll... Aleksandr Urakov via Phabricator via lldb-commits
  • [Ll... Aleksandr Urakov via Phabricator via lldb-commits
    • ... Zachary Turner via lldb-commits
      • ... Mailing List "llvm-commits" via Phabricator via lldb-commits

Reply via email to