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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: test/CodeGen/debug-info-enum.cpp:2
+// RUN: %clang -target x86_64-linux -g -S -emit-llvm -o - %s | FileCheck %s
+enum class E0 : signed char {
+  A0 = -128,
----------------
Could you summarize the purpose of each of these tests (possibly in a comment above the enum in each case) - there look to be more test cases than I'd imagine being necessary, but I haven't carefully analyzed them.

For example: I wouldn't expect to test every integer type, if the code handling them is general enough to be demonstrated by one or two cases?


================
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)
----------------
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. 


================
Comment at: test/CodeGen/debug-info-enum.cpp:66
+// CHECK-SAME: baseType: ![[INT]]
+// CHECK-NOT: flags: DIFlagFixedEnum
+// CHECK: !DIEnumerator(name: "A8", value: -128)
----------------
Probably drop the "flags: " part of this NOT check - so that if other flags are added, this won't be overly constrained (eg: if this type ended up with "flags: X | DIFlagFixedEnum" this NOT check as-is wouldn't catch the regression)


https://reviews.llvm.org/D42736