[PATCH] D63475: [llvm-readobj] Allow --hex-dump/--string-dump to dump multiple sections

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-readobj/hex-dump.test:29
+
+
+# ELF-SEC: [ 9] .strtab
----------------
Unnecessary extra line here?


================
Comment at: tools/llvm-readobj/ObjDumper.cpp:38
+getSectionRefsByNameOrIndex(const object::ObjectFile *Obj,
+                            const std::vector<std::string> &Sections) {
+  std::vector<object::SectionRef> Ret;
----------------
Could Sections be an `ArrayRef`?


================
Comment at: tools/llvm-readobj/ObjDumper.cpp:40
+  std::vector<object::SectionRef> Ret;
+  std::map<std::string, bool> SecNames;
+  std::map<int, bool> SecIndices;
----------------
I may be being dumb, but why can't we use a StringMap? I'm not seeing where the ordering makes a difference. Or is it just that the output order might change arbitrarily?


================
Comment at: tools/llvm-readobj/ObjDumper.cpp:44
+  for (StringRef Section : Sections) {
+    if (!Section.getAsInteger(0, SecIndex))
+      SecIndices.emplace(SecIndex, false);
----------------
I don't think we want to accept negative numbers as section indices here. Can you change the type of SecIndex to an unsigned type? I think that will cause this to become an unsigned lookup.

Then again, using a signed type is probably harmless, since we'll warn later, it just is confusing looking at the code as is.


================
Comment at: tools/llvm-readobj/ObjDumper.cpp:55
+      error(EC);
       continue;
     }
----------------
Is the `continue` necessary? I think `error()` ends the program...


================
Comment at: tools/llvm-readobj/ObjDumper.cpp:57
     }
-    W.startLine() << format("[%6tx] ", CurrentWord - SecContent);
-    printAsPrintable(W.startLine(), CurrentWord, WordSize);
-    W.startLine() << '\n';
-    CurrentWord += WordSize + 1;
+    auto It = SecNames.find(SecName);
+    if (It != SecNames.end())
----------------
Maybe `NameIt` and `IndexIt` instead of `It` and `It2` might be a little clearer.


================
Comment at: tools/llvm-readobj/ObjDumper.cpp:79
+void ObjDumper::printSectionsAsString(
+    const object::ObjectFile *Obj, const std::vector<std::string> &Sections) {
+  bool First = true;
----------------
`ArrayRef`?


================
Comment at: tools/llvm-readobj/ObjDumper.cpp:113
+void ObjDumper::printSectionsAsHex(const object::ObjectFile *Obj,
+                                   const std::vector<std::string> &Sections) {
+  bool First = true;
----------------
`ArrayRef`?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63475