[PATCH] D13038: dsymutil: Resolve forward decls for types defined in clang modules.

[Frederic Riss via llvm-commits]

friss added a comment.

Basically looks good. Some comments/questions:


================
Comment at: tools/dsymutil/DwarfLinker.cpp:1667-1672
@@ -1656,7 +1666,8 @@
+  
   // FIXME: dsymutil-classic compat won't unique the same type
   // presented once as a struct and once as a class. Use the Tag in
   // the fully qualified name hash to get the same effect.
   // We hash NameRef, which is the mangled name, in order to get most
   // overloaded functions resolvec correctly.
   unsigned Hash = hash_combine(Context.getQualifiedNameHash(), Tag, NameRef);
 
----------------
I like the simplicity of adding modules as some kind of namespace. The fact that we hash the Tag here prevents us from uniquing what's in a module and standard namespaces. However the FIXME above only talks about backward compatibility. Maybe we should remove that FIXME and instead explain that it avoid conflicts between modules and other Decl scopes (and add a small note about preventing uniquing a struct with a class of the same name, but that this corner case doesn't warrant making that conditional).

================
Comment at: tools/dsymutil/DwarfLinker.cpp:1751-1753
@@ -1739,5 +1750,5 @@
 
 /// \brief Recursive helper to gather the child->parent relationships in the
 /// original compile unit.
-static void gatherDIEParents(const DWARFDebugInfoEntryMinimal *DIE,
+static bool gatherDIEParents(const DWARFDebugInfoEntryMinimal *DIE,
                              unsigned ParentIdx, CompileUnit &CU,
----------------
We should really rename that and change the comment (it should have done before, but your patch makes it even more meaningless). Maybe gatherInitialDIEInfo ? Or gatherDIEScopes ? Pick whichever you want (or an even more awesome name that I couldn't come up with).

================
Comment at: tools/dsymutil/DwarfLinker.cpp:1796-1801
@@ +1795,8 @@
+
+  // Prune this DIE if it is either a forward declaration inside a
+  // DW_TAG_module or a DW_TAG_module that contains nothing but
+  // forward declarations.
+  Info.Prune &= (DIE->getTag() == dwarf::DW_TAG_module) ||
+                DIE->getAttributeValueAsUnsignedConstant(
+                    &CU.getOrigUnit(), dwarf::DW_AT_declaration, 0);
+
----------------
Wouldn't that logic always prune the TAG_module? or am I reading this wrong?

================
Comment at: tools/dsymutil/DwarfLinker.cpp:2214-2215
@@ -2176,2 +2213,4 @@
   bool AlreadyKept = MyInfo.Keep;
+  if (MyInfo.Prune && !AlreadyKept)
+    return;
 
----------------
I would have expected Prune to override Keep. What is the '&& !AlreadyKept' handling?


http://reviews.llvm.org/D13038