[PATCH] D66405: [llvm-ifs] llvm Interface Stubs merging + object file generation tool.

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added a comment.

This definitely needs tests.



================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:53
+
+const VersionTuple IFSVersionCurrent(1, 0);
+template <> struct llvm::yaml::ScalarTraits<VersionTuple> {
----------------
Hmm, put this into an anonymous namespace and hoist it to the top of the file?


================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:86
+      IO.mapRequired("Size", Symbol.Size);
+    }
+    IO.mapOptional("Undefined", Symbol.Undefined, false);
----------------
Braces are unnecessary.


================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:100
+    ELFSymbol Sym(Key.str());
+    IO.mapRequired(Key.str().c_str(), Sym);
+    Set.insert(Sym);
----------------
Use a local variable for the `Key.str()`.  You are creating two `std::string` here otherwise.


================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:125
+      : IfsVersion(Stub.IfsVersion), Triple(Stub.Triple),
+        ObjectFileFormat(Stub.ObjectFileFormat), SoName(Stub.SoName),
+        NeededLibs(Stub.NeededLibs), Symbols(Stub.Symbols) {}
----------------
`SOName` is better - its an initialism for `Shared Object`


================
Comment at: llvm/tools/llvm-ifs/llvm-ifs.cpp:265
+    for (auto Symbol : TargetStub->Symbols)
+      Stub.Symbols.insert(Symbol);
+  }
----------------
What about conflicts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66405