[PATCH] D74205: [llvm-dwarfdump] Add the --show-sections-sizes option

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes_no_debug_sections.test:6
+# CHECK-NEXT:----------------------------------------------------
+# CHECK-NEXT:No debug sections found.
+# CHECK-NEXT:----------------------------------------------------
----------------
Rather than having a special message here, I'd probably just print the Total Size and Total File Size lines as before (which will be 0 and the object file size).


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/section_sizes_no_debug_sections.test:16-21
+  - Name:         .foo
+    Type:         SHT_PROGBITS
+    Size:         17
+  - Name:         .bar
+    Type:         SHT_PROGBITS
+    Size:         19
----------------
Remove the extra spaces between the keys and values.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:15
+
+namespace llvm {
+
----------------
No need for the namespace here, since you are `using namespace llvm` above.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:31
+size_t getSizeColumnWidth(const SectionSizes &Sizes,
+                          const StringRef SecttonSizeTitle) {
+  // The minimum column width should be the size of "SIZE".
----------------
Typo: `SectionSizeTitle`


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.h:32-37
+/// Get the maximum section name width of the found debug sections.
+size_t getMaxSectionNameWidth(const SectionSizes &Sizes,
+                              const StringRef SectionNameColPrintName);
+
+/// Get the maximum section size bytes number width of the found debug sections.
+size_t getMaxSectionSizeNumberWidth(const SectionSizes &Sizes,
----------------
It looks like these two functions are no longer defined after you renamed them in the source file? If they aren't used outside the source file, just make them static functions in the .cpp.

Same goes for everything in here - only include in the header what needs to be part of the public interface.


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

https://reviews.llvm.org/D74205