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

[Armando Montanez via Phabricator via llvm-commits]

amontanez added inline comments.


================
Comment at: llvm/include/llvm/TAPI/ELFInterfaceFile.h:43
+
+class ELFInterfaceFile {
+public:
----------------
jakehehrlich wrote:
> 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.
Discussed offline, removed this since it provides abstraction that isn't clear will be necessary.


================
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);
+};
----------------
jakehehrlich wrote:
> 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.
Noted. It's unclear what that will look like right now, so I'll keep this as-is until we reach the point that this becomes a concern.


Repository:
  rL LLVM

https://reviews.llvm.org/D53051