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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: llvm/include/llvm/TAPI/ELFInterfaceFile.h:43
+
+class ELFInterfaceFile {
+public:
----------------
Can you give a clear outline as to what the ELFInterfaceFile class does that ELFStub does not? At the moment it seems to merely be an extremely verbose way of keeping track of FileType manually. In llvm-objcopy we have to keep track of this because there is an expectation that the output type match the input type but I don't think you have that constraint here. One output type is assumed if none is given, .tbe.

I think a better way to handle the different formats is to have a ReaderWriter class for each and methods format. That can then provide the description, and those can be registered in the command line tool in an appropriate fashion. The user should be able to know which type of object they read in by checking which reader/writer they used successfully.


================
Comment at: llvm/include/llvm/TAPI/ELFInterfaceFile.h:45
+public:
+  explicit ELFInterfaceFile(FileType Type) : Type(Type) {}
+  explicit ELFInterfaceFile(FileType Type, ELFStub &SourceStub)
----------------
What is the intended semantics of a interface without a stub?


================
Comment at: llvm/include/llvm/TAPI/ELFStub.h:26
+
+enum ELFSymbolType {
+  NoType = ELF::STT_NOTYPE,
----------------
Can this be an enum class or "scoped enum"? Same for all enums.


================
Comment at: llvm/include/llvm/TAPI/ELFStub.h:40
+  ELFSymbolType Type;
+  bool Undefined;
+  Optional<std::string> Warning;
----------------
We decided offline that we needed to keep track of weakness correct?


================
Comment at: llvm/include/llvm/TAPI/ELFStub.h:46
+// Both textual and binary stubs will read into and write from this object.
+class ELFStub {
+public:
----------------
offline we decided we needed to keep track of dt_needed as well, right? I'll accept a TODO here for that.


================
Comment at: llvm/include/llvm/TAPI/ELFStub.h:55
+
+  static bool symbolCompareTo(const ELFSymbol &LHS, const ELFSymbol &RHS) {
+    return LHS.Name < RHS.Name;
----------------
Where ever you use this, you can most likely replace it with a lambda or a direct comparison.


================
Comment at: llvm/include/llvm/TAPI/TBEHandler.h:37
+  /// Attempts to write an ELF interface file to a raw_ostream.
+  Error writeFile(raw_ostream &OS, const ELFInterfaceFile *File);
+};
----------------
I don't think using a raw_ostream is what we want here. It's probably fine but it would be better to use some kind of abstraction over Buffers that allowed both a raw_ostream to be used under the hood *and* a MemoryBuffer or FileBuffer. Importantly you avoid a copy of the file by using a FileBuffer on most systems.


================
Comment at: llvm/lib/TAPI/ELFInterfaceFile.cpp:18
+Error ELFInterfaceFile::setFileType(FileType NewType) {
+  assert(NewType != FileType::All && "Can't set file type to FileType::All!");
+  if (!supportsType(NewType)) {
----------------
The reality is that you can't set to any non-single value.


================
Comment at: llvm/lib/TAPI/ELFInterfaceFile.cpp:31
+  switch (Type) {
+  case FileType::Invalid:
+    return "invalid";
----------------
An Invalid FileType doesn't make sense. Why do you need this?


================
Comment at: llvm/lib/TAPI/ELFInterfaceFile.cpp:43
+    return "text-based ELF stub v1";
+  case FileType::All:
+    return "all file types";
----------------
All should probably not be an enum. The FileType enum is currently mixing up the notion of a value with a notion of a set IMO.


================
Comment at: llvm/lib/TAPI/ELFStub.cpp:15
+
+ELFStub::ELFStub(ELFStub const &Stub) {
+  Arch = Stub.Arch;
----------------
Define move constructor as well.


================
Comment at: llvm/lib/TAPI/ELFStub.cpp:19
+  for (auto Elem : Stub.Symbols) {
+    Symbols.push_back(Elem);
+  }
----------------
Use '=' instead of loop and push_back.


Repository:
  rL LLVM

https://reviews.llvm.org/D53051