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

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi added inline comments.


================
Comment at: llvm/include/llvm/InterfaceStub/ELFStub.h:90
+// structure can be used for 2 different yaml schema.
+class ELFStubTriple : public ELFStub {
+public:
----------------
struct instead of class public? 


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:564
   // Populate Arch from ELF header.
-  DestStub->Arch = ElfFile.getHeader().e_machine;
+  DestStub->Target.Arch = (ELFArch)(uint16_t)(ElfFile.getHeader().e_machine);
+  DestStub->Target.BitWidth =
----------------
Why both casts? How about one cast, or a static_cast<ELFArch> ? 


================
Comment at: llvm/lib/InterfaceStub/TBEHandler.cpp:50
       break;
-    case (ELFArch)ELF::EM_NONE:
-    default:
-      Out << "Unknown";
     }
   }
----------------
default llvm_unreachable here? 


================
Comment at: llvm/lib/InterfaceStub/TBEHandler.cpp:54
+  static StringRef input(StringRef Scalar, void *, ELFEndiannessType &Value) {
+    Value = StringSwitch<ELFEndiannessType>(Scalar)
+                .Case("big", ELFEndiannessType::Big)
----------------
Default StringSwitch case? 


================
Comment at: llvm/lib/InterfaceStub/TBEHandler.cpp:73
+      break;
+    }
+  }
----------------
default llvm_unreachable?


================
Comment at: llvm/lib/InterfaceStub/TBEHandler.cpp:79
+                .Case("32", ELFBitWidthType::ELF32)
+                .Case("64", ELFBitWidthType::ELF64);
     return StringRef();
----------------
Same as before. Default? 


================
Comment at: llvm/lib/InterfaceStub/TBEHandler.cpp:198
+  else
+    YamlOut << *dynamic_cast<ELFStub *>(CopyStub.get());
+  return Error::success();
----------------
Do we even support dynamic_casts in LLVM anymore? I might be missing something here, sorry. 


================
Comment at: llvm/lib/InterfaceStub/TBEHandler.cpp:138
+      IO.setError("Not a .tbe YAML file.");
+    IO.mapRequired("TbeVersion", Stub.TbeVersion);
+    IO.mapOptional("SoName", Stub.SoName);
----------------
phosek wrote:
> Would this eventually become `IFSVersion`?
I think I would be OK with IFSVersion and TBE version sharing the same number. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99399