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

[Frederic Riss via Phabricator via llvm-commits]

friss added a comment.

A few nits, but this mostly LGTM



================
Comment at: test/tools/dsymutil/X86/odr-fwd-declaration2.cpp:26
+// CHECK: DW_TAG_member
+// CHECK: AT_name{{.*}} "bPtr"
+// CHECK-NOT: {{DW_TAG|NULL}}
----------------
Add a CHECK-NOT: {{DW_TAG|NULL}} before this


================
Comment at: test/tools/dsymutil/X86/odr-fwd-declaration2.cpp:85
+// CHECK: DW_TAG_member
+// CHECK: AT_name{{.*}} "bPtr"
+// CHECK-NOT: {{DW_TAG|NULL}}
----------------
Add the same CHECK-NOT here


================
Comment at: test/tools/dsymutil/X86/odr-fwd-declaration2.cpp:98
+// CHECK-NOT: {{DW_TAG|NULL}}
+// CHECK: AT_type{{.*}}0x{{0*}}[[REF1:[a-f0-9]*]]
+//
----------------
You are reusing REF1. I suppose this those the right thing (overwrites it), but please use a different name.


================
Comment at: test/tools/dsymutil/X86/odr-fwd-declaration2.cpp:134-135
+
+// Finally we confirm that uniquing is not broken and bPtr, bRef and
+// bPtrToField still reference the same definition of B.
+//
----------------
As the bellow doesn't really test bPTr, bRef and bPtrToField, I would rephrase this as:
"Finally we confirm that uniquing isn't broken by checking that further references to 'struct A' point to its now complete definition.


================
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?
   };
----------------
aprantl wrote:
> is there a more accurate description that "point to"? I.e., does this include a struct that has a fwddecl as a member?
If it fits, "Does this DIE transitively refer to an incomplete decl?" would be better I think


Repository:
  rL LLVM

https://reviews.llvm.org/D37127