[PATCH] D37127: [dsymutil] Don't mark forward declarations as canonical.

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

mostly nitpicking :-)



================
Comment at: test/tools/dsymutil/X86/odr-fwd-declaration.cpp:113
+
+// Finally we confirm that uniquing is not borken and the same typedef is
+// referneced by ptr1.
----------------
broken


================
Comment at: test/tools/dsymutil/X86/odr-fwd-declaration.cpp:114
+// Finally we confirm that uniquing is not borken and the same typedef is
+// referneced by ptr1.
+//
----------------
referenced


================
Comment at: tools/dsymutil/DwarfLinker.cpp:195
+    bool Prune : 1;      ///< Is this a pure forward declaration we can strip?
+    bool Incomplete : 1; ///< Does this DIE point to an forward decl?
   };
----------------
is there a more accurate description that "point to"? I.e., does this include a struct that has a fwddecl as a member?


================
Comment at: tools/dsymutil/DwarfLinker.cpp:1202
   /// keep. Store that information in \p CU's DIEInfo.
-  void lookForDIEsToKeep(RelocationManager &RelocMgr,
-                         const DWARFDie &DIE,
+  bool lookForDIEsToKeep(RelocationManager &RelocMgr, const DWARFDie &DIE,
                          const DebugMapObject &DMO, CompileUnit &CU,
----------------
document the return value?


================
Comment at: tools/dsymutil/DwarfLinker.cpp:2210
                       TF_ParentWalk | TF_Keep | TF_DependencyWalk | ODRFlag);
+
     AncestorIdx = CU.getInfo(AncestorIdx).ParentIdx;
----------------
extra newline? not sure if this was intentional


================
Comment at: tools/dsymutil/DwarfLinker.cpp:2261
+
+      // If the current DIE is not incomplete, but it references an incomplete
+      // one we propagate the incompleteness.
----------------
maybe `is complete`?


================
Comment at: tools/dsymutil/DwarfLinker.cpp:2286
 /// traversal we are currently doing.
-void DwarfLinker::lookForDIEsToKeep(RelocationManager &RelocMgr,
+bool DwarfLinker::lookForDIEsToKeep(RelocationManager &RelocMgr,
                                     const DWARFDie &Die,
----------------
what does the return value mean?


Repository:
  rL LLVM

https://reviews.llvm.org/D37127