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

[Adrian Prantl via llvm-commits]

aprantl added inline comments.

================
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);
 
----------------
friss wrote:
> 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).

> The fact that we hash the Tag here prevents us from uniquing what's in a module and standard namespaces.
Even if we wouldn't hash the Tag, we would still hash the module's name (NameRef) which is crucial in order to support (Obj)C because it is legal to have conflicting definitions for the same type in two differently named modules.

In order to support uniquing in C++ between module types and other types, we should change clang to not emit C++ types inside a DW_TAG_module instead.

================
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,
----------------
friss wrote:
> 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).
I tried analyzeContextInfo(). We can still rename it if we can come up with something better.


http://reviews.llvm.org/D13038