[PATCH] D48281: [llvm-readobj] Add -hex-dump (-x) option

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/Object/MachO.h:307-309
+  Expected<SectionRef> getSection(unsigned SectionIndex) const;
+  std::error_code getSection(StringRef SectionName,
+                             const SectionRef *&Result) const;
----------------
Is there really no suitable existing API for getting the contents of a section? (like the ones above that use DataRefImpl?)

& these two APIs using different schemes (one returning Expected<SectionRef> the other returning error_code and taking a SectionRef out parameter) seem unnecessarily inconsistent - can they use a matching approach (I'd favor the Expected<SectionRef> option)?

Maybe @echristo has some ideas/familiarity here.


================
Comment at: tools/llvm-readobj/COFFDumper.cpp:665
+  else {
+    error(Obj->getSection((int)SectionIndex, Sec));
+    if (!Sec)
----------------
Is there a need for bounds checking before casting that long down to an integer? (what's the intended behavior when the value is too large to fit in an integer? Is that tested?)


================
Comment at: tools/llvm-readobj/COFFDumper.cpp:697
+    // Then, the (4 - i) spaces that are in between the rows.
+    // Least, if we cut in a middle of a row, we add the remaining characters,
+    // which is (8 - (k * 2))
----------------
Typo 'Least' -> 'Last' (or Lastly)?


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:153
   void printSectionAsString(StringRef StringName) override;
+  void printSectionAsHex(StringRef StringName) override;
   void printHashTable() override;
----------------
Does this need support in each format explicitly - or is there a way to generalize it over all formats in one place? (ie: could all formats support the ability to retrieve the section contents, and then the printing would be generalized over that)

Certainly looks like there's a lot of duplication in the different dumper function implementations - at least would be good to pull a lot of that out into a generalized helper even if there is some format-specific stuff to be handled (though I'm not sure if there should be any format-specific stuff to handle).


Repository:
  rL LLVM

https://reviews.llvm.org/D48281