[PATCH] [ dwarfdump ] Add symbolic dump of known DWARF attribute values.

[David Blaikie]

================
Comment at: lib/DebugInfo/DWARFDebugInfoEntry.cpp:90
@@ +89,3 @@
+  
+  Optional<uint64_t> Val = formValue.getAsUnsignedConstant();
+  const char *Name = nullptr;
----------------
Roll the "Val" variable into the if condition to reduce its scope:

if (Optional<uint64_t> Val = formValue.getAsUnsignedConstant())
  Name = AttributeValueString(attr, Val.getValue());

(personally I prefer using the pointer-like interface of Optional (so I would write "*Val" rather than "Val.getValue()") but opinions differ and there's no solid convention in the codebase so far - so do whichever you prefer, I just mention it in case you aren't aware of it)

================
Comment at: lib/DebugInfo/DWARFDebugInfoEntry.cpp:96
@@ +95,3 @@
+  if (Name) {
+      OS << Name;
+  } else {
----------------
Mistaken indentation here? (looks like a 4 space instead of a 2, but it might be the review software being weird)

================
Comment at: lib/Support/Dwarf.cpp:799
@@ +798,3 @@
+
+const char *llvm::dwarf::AttributeValueString(uint16_t Attr, unsigned Val) {
+  switch (Attr) {
----------------
As a follow-up, if you like, all these functions should /probably/ be modified to return StringRef, that way we don't have to compute their length later on (or do null-test walks) in printing functions, etc.

================
Comment at: lib/Support/Dwarf.cpp:799
@@ +798,3 @@
+
+const char *llvm::dwarf::AttributeValueString(uint16_t Attr, unsigned Val) {
+  switch (Attr) {
----------------
dblaikie wrote:
> As a follow-up, if you like, all these functions should /probably/ be modified to return StringRef, that way we don't have to compute their length later on (or do null-test walks) in printing functions, etc.
Should the type of Attr be something more type-safe? (the actual enum of DWARF attributes)

================
Comment at: lib/Support/Dwarf.cpp:829
@@ +828,3 @@
+
+  return nullptr;
+}
----------------
This line is unreachable (so far as I can see?) and should be deleted.

(alternatively you could leave this one and remove the default case from the switch)

http://reviews.llvm.org/D5187