[PATCH] D60321: [llvm-readobj] Add helper functions `printVersionSymbol()`, `printVersionDefinition()` and `printVersionDependency()`

[James Henderson via Phabricator via llvm-commits]

jhenderson added reviewers: grimar, rupprecht.
jhenderson added a comment.

As I'll be away next week, I've added a couple of others to help with reviewing this too.

I'm not certain that the approach you've taken is quite right. There should be no need to query the `opts::Output` member from within the dumper. That's the purpose of the individual dump styles. There's barely any common code you've actually pulled out. What is the purpose of what you have done?



================
Comment at: llvm/test/tools/llvm-readobj/elf-empty-version-info.test:2
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readobj -V %t | FileCheck %s
+
----------------
What about a test for the llvm-readelf behaviour?


================
Comment at: llvm/test/tools/llvm-readobj/elf-empty-version-info.test:19-21
+Sections:
+  - Name: .text
+    Type: SHT_PROGBITS
----------------
Can you get rid of the Sections: block?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:354
                                    cl::boolOrDefault PrintSectionMapping) = 0;
-  virtual void printVersionSymbolSection(const ELFFile<ELFT> *Obj,
-                                         const Elf_Shdr *Sec) = 0;
-  virtual void printVersionDefinitionSection(const ELFFile<ELFT> *Obj,
-                                             const Elf_Shdr *Sec) = 0;
-  virtual void printVersionDependencySection(const ELFFile<ELFT> *Obj,
-                                             const Elf_Shdr *Sec) = 0;
+  virtual void printVersionSymbol(const ELFFile<ELFT> *Obj, const Elf_Shdr *Sec,
+                                  StringRef SecName, StringRef StrTab,
----------------
This should be plural (i.e. printVersionSymbols) since it will print more than one symbol. The same goes for the other functions below (Definitions and Dependencies).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60321