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

[Eric Christopher via Phabricator via llvm-commits]

echristo 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;
----------------
dblaikie wrote:
> 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.
In particular, I'd have thought we wanted getSectionContents here?


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:153
   void printSectionAsString(StringRef StringName) override;
+  void printSectionAsHex(StringRef StringName) override;
   void printHashTable() override;
----------------
dblaikie wrote:
> 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).
+1 


Repository:
  rL LLVM

https://reviews.llvm.org/D48281