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

[Frederic Riss]

================
Comment at: lib/DebugInfo/DWARFDebugInfoEntry.cpp:90
@@ +89,3 @@
+  
+  Optional<uint64_t> Val = formValue.getAsUnsignedConstant();
+  const char *Name = nullptr;
----------------
dblaikie wrote:
> 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)
Will do. I wasn't aware of Optional::operator*. I'll use it.

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

================
Comment at: lib/Support/Dwarf.cpp:799
@@ +798,3 @@
+
+const char *llvm::dwarf::AttributeValueString(uint16_t Attr, unsigned Val) {
+  switch (Attr) {
----------------
dblaikie wrote:
> 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)
I first wrote it like that, but I was finding the cast at the call site awkward. I can add it back, but we have no guarantee that the data we've read from the DWARF file will actually be a known Attribute, thus I find it more accurate to allow all values.

================
Comment at: lib/Support/Dwarf.cpp:829
@@ +828,3 @@
+
+  return nullptr;
+}
----------------
dblaikie wrote:
> 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)
I actually wondered while writing the code if all supported compilers could correctly do the control flow analysis that proves that the second return is unnecessary. I added the default case to prevent warnings about missed cases when I was using a typed enum Attr, but now the default case can just go away.

http://reviews.llvm.org/D5187