[Lldb-commits] [PATCH] D54357: [NativePDB] Improved support for nested type reconstruction

[Aleksandr Urakov via Phabricator via lldb-commits]

aleksandr.urakov added a comment.

This change looks reasonable to me for solving the problem with the current `LF_NESTTYPE` approach. But I'm not sure... Now we all the same need to analyze mangled names and make a decision based on this. Does the current `LF_NESTTYPE` approach still has advantages over the "parse-scope" approach? I'm afraid that we will have to fully analyze a mangled name in the future for scoped types, so if the `LF_NESTTYPE` approach doesn't have a significant advantages over the "parse-scope" approach, do we really need to support them both?



================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:560-576
+  // If it's an inner definition, then treat whatever name we have here as a
+  // single component of a mangled name.  So we can inject it into the parent's
+  // mangled name to see if it matches.
+  CVTagRecord child = CVTagRecord::create(tpi.getType(Record.Type));
+  std::string qname = parent.asTag().getUniqueName();
+  if (qname.size() < 4 || child.asTag().getUniqueName().size() < 4)
+    return llvm::None;
----------------
May be it would be a good idea to make the demangler (or to implement a mangler) more flexible to hide these details there? I do not mean to do it exactly in this commit, but what do you think about this at all?


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:881-887
+    // If there is no parent in the debug info, but some of the scopes have
+    // template params, then this is a case of bad debug info.  See, for
+    // example, llvm.org/pr39607.  We don't want to create an ambiguity between
+    // a NamespaceDecl and a CXXRecordDecl, so instead we create a class at
+    // global scope with the fully qualified name.
+    if (AnyScopesHaveTemplateParams(scopes))
+      return {context, record.Name};
----------------
Is this a clang only bug, or MSVC also does so? I do not have anything against this solution for now, I just mean, may be we'll replace it with an `assert` when the bug will be fixed?


https://reviews.llvm.org/D54357