[PATCH] D58763: [llvm-objdump] Should print unknown d_tag in hex format

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, assuming the case printing is correct, and the two other small comments are addressed.



================
Comment at: lib/Object/ELF.cpp:473
   default:
-    return "unknown";
+    return "<unknown:>0x" + utohexstr(Type, true);
   }
----------------
This prints the hex digits as lower-case. Does that match what the rest of the llvm-objdump printing does?


================
Comment at: test/tools/llvm-objdump/elf-dynamic-section.test:61
 # CHECK-NEXT:   FILTER               U
+# CHECK-NEXT:   <unknown:>0x12345678 0x0000000000000001
 
----------------
Maybe worth adding a hex digit to the tag value, to show upper/lower case behaviour.


================
Comment at: tools/llvm-objdump/ELFDump.cpp:179
 
-    StringRef Str = StringRef(Elf->getDynamicTagAsString(Dyn.d_tag));
-
-    if (Str.empty()) {
-      std::string HexStr = utohexstr(static_cast<uint64_t>(Dyn.d_tag), true);
-      outs() << format("  0x%-19s", HexStr.c_str());
-    } else {
-      // We use "-21" in order to match GNU objdump's output.
-      outs() << format("  %-21s", Str.data());
-    }
+    auto Str = Elf->getDynamicTagAsString(Dyn.d_tag);
+    outs() << format("  %-21s", Str.c_str());
----------------
As we're messing about with different string types (const char *, std::string, StringRef etc), it's probably best to not use auto here.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58763/new/

https://reviews.llvm.org/D58763