[PATCH] D72992: [llvm-objdump] - Add column headers for relocation printing

Mon Jan 20 03:03:03 PST 2020

jhenderson added a comment.

Thanks for the patch!

It looks like you haven't run the tests. Please make sure to run the check-all target, to run all the tests. In particular, I expect to see many tests that will fail, because they do not expect the extra line for the headers. Also, please make sure to update the llvm/test/tools/llvm-objdump/elf-relocations.test to check this works. You will need to add a 32-bit test case probably, to show that the 16 versus 8 space padding decision is made correctly. If you need help writing these tests, please let me know, and I'll be happy to help out.

================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1642-1643
     outs() << "RELOCATION RECORDS FOR [" << SecName << "]:\n";
+    uint32_t offset_padding = (Obj->getBytesInAddress() > 4 ? 16 : 8);
+    uint32_t type_padding = 16;
+    outs() << left_justify("OFFSET", offset_padding) << " "
----------------
LLVM's [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | normal variable naming style ]] is UpperCamelCase, so this should be OffsetPadding, and TypePadding.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72992