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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.

Looks good - thanks!



================
Comment at: lib/AsmParser/LLParser.cpp:3939-3943
+    if (!ParseMDField(Loc, Name, Res)) {
+      Result.assign(Res);
+      return false;
+    }
+    return true;
----------------
Could invert this (& similarly below) to reduce indentation & bracing:

  if (ParseMDField(...))
    return true;
  Result.assign(Res);
  return false;


================
Comment at: test/Assembler/DIEnumerator.ll:24-64
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
+!1 = distinct !DIGlobalVariable(name: "x0", scope: !2, file: !3, line: 1, type: !5, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, producer: "clang version 7.0.0 (/data/src/llvm/tools/clang 697b0cb4c2e712a28767c2f7fe50c90bae7255f5) (/data/src/llvm 5ba8dcca7470b5da405bc92b9681b1f36e5d6772)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, globals: !23)
+!3 = !DIFile(filename: "e.cc", directory: "/work/build/clang-dev")
+!4 = !{!5, !10, !14, !19}
+!5 = !DICompositeType(tag: DW_TAG_enumeration_type, name: "E0", file: !3, line: 1, baseType: !6, size: 32, elements: !7, identifier: "_ZTS2E0")
+; CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E0"{{.*}}, baseType: ![[INT:[0-9]+]]
----------------
Probably add some whitespace in here at the beginning of each enum so it's easier to read/visually separate each test case?


================
Comment at: test/DebugInfo/Generic/debug-info-enum.ll:24
+!4 = !{!5, !10, !14, !19, !23, !28, !32, !37, !41}
+!5 = !DICompositeType(tag: DW_TAG_enumeration_type, name: "E0", file: !3, line: 2, baseType: !6, size: 8, flags: DIFlagFixedEnum, elements: !7, identifier: "_ZTS2E0")
+!6 = !DIBasicType(name: "signed char", size: 8, encoding: DW_ATE_signed_char)
----------------
Worth a comment at the start of each enumeration/test case describing what this test covers that the others don't? So it's clear what to focus on? (just a short, english language (rather than metadata/check lines) summary of the important features "test a fixed enum class over signed char" or the like)


https://reviews.llvm.org/D42734