[PATCH] D42736: [DebugInfo] Improvements to representation of enumeration types (PR36168)

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.

Few things that could tidy up the tests a bit, but otherwise looks good :)



================
Comment at: test/CodeGen/debug-info-enum.cpp:6-10
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E0"
+// CHECK-SAME: flags: DIFlagFixedEnum
+// CHECK: !DIBasicType(name: "signed char", size: 8, encoding: DW_ATE_signed_char)
+// CHECK: !DIEnumerator(name: "A0", value: -128)
+// CHECK: !DIEnumerator(name: "B0", value: 127)
----------------
chill wrote:
> dblaikie wrote:
> > Rather than relying on specific ordering of output, generally you should test that the actual references across different metadata records are correct (eg: check that the DICompositeType's member list elements are the DIEnumerators (use named matches, rather than hardcoding the metadata node numbers))
> > 
> > Though, admittedly, this is a lot easier to read as-is. 
> I've done that, to some extent, making sure each enumeration refers to the expected underlying type. Not idea how to do that completely independent of order.
Thanks! Maybe add a comment (like I suggested in the LLVM review too, just now) in each case to describe each test case. You have one up the top for the enum class over signed char, but someting similar for each enum might be good.

Yeah, it's impossible to do it entirely independent of order.

You could do a bit more than what you've got here - the DIEnumerators could be checked:

match the list from the DICompositeType enumeration_type (the same way you match the baseType) then match the elements of the list, then match those elements to the DIEnumerators same as you do for the DIBasicType) - does that make sense?


https://reviews.llvm.org/D42736