[PATCH] D53051: [llvm-tapi] initial commit, supports ELF text stubs

[Juergen Ributzka via Phabricator via llvm-commits]

ributzka accepted this revision.
ributzka added inline comments.


================
Comment at: llvm/include/llvm/TextAPI/ELF/ELFStub.h:23
+namespace llvm {
+namespace tapi {
+namespace elf {
----------------
I don't think the tapi namespace is required in general for this library, but that is just a personal preference.


================
Comment at: llvm/lib/CMakeLists.txt:26
 add_subdirectory(Passes)
+add_subdirectory(TextAPI)
 add_subdirectory(ToolDrivers)
----------------
Thanks for changing the name. This will make it easier to rebase my patch.


================
Comment at: llvm/lib/TextAPI/ELF/TBEHandler.cpp:44
+    // Map from integer to architecture string.
+    static const DenseMap<uint16_t, StringRef> ArchMap = {
+        {(uint16_t)ELF::EM_X86_64, "x86_64"},
----------------
Why do you use a DenseMap here? Wouldn't be a switch enough?


================
Comment at: llvm/lib/TextAPI/ELF/TBEHandler.cpp:57
+    // Map from architecture string to integer.
+    static const StringMap<uint16_t> ArchMap = {
+        {"x86_64", (uint16_t)ELF::EM_X86_64},
----------------
StringSwitch from ADT would be an alternative here too.


Repository:
  rL LLVM

https://reviews.llvm.org/D53051