[PATCH] D99399: [elfabi] Prepare llvm-elfabi for elfabi/ifs merging.

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: llvm/lib/BinaryFormat/ELF.cpp:206
+std::string ELF::convertEMachineToArchName(uint16_t EMachine) {
+  DenseMap<uint16_t, std::string> EMachineMap = {
+      {ELF::EM_NONE, "None"},
----------------
I'd just use `switch` and return `StringRef` akin to https://github.com/llvm/llvm-project/blob/f59ba0849f7a148265dcc89d36a11717d365f368/llvm/lib/Support/Triple.cpp#L23


================
Comment at: llvm/lib/InterfaceStub/ELFStub.cpp:58
+Expected<ELFArch> llvm::elfabi::convertArchNameToEMachine(StringRef Arch) {
+  static StringMap<uint16_t> ArchNameMap = {
+      {"none", ELF::EM_NONE},
----------------
haowei wrote:
> phosek wrote:
> > `StringSwitch` should be more efficient than `StringMap`. It may be better to move this to `llvm/lib/BinaryFormat/ELF.cpp` and expose it as a helper function through `llvm/include/llvm/BinaryFormat/ELF.h`.
> 
> > StringSwitch should be more efficient than StringMap. 
> 
> Is that because the map is relatively small, nor worth hashing it?
> 
> 
`StringSwitch` is written in a way that takes advantage of LLVM optimizations and should produce the most optimal code in this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99399