[PATCH] D48413: [llvm-objcopy] Add initial support for static libraries

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm wondering if we should have a basic thin archive member test too? Their format is sufficiently different that it could expose some interesting code paths in llvm-objcopy (I'm assuming that it won't actually matter in our current case, as I'm expecting the archive library to abstract that away, but I could imagine some differences in how things are written out). What do you think?

@jakehehrlich - any comments from you?



================
Comment at: tools/llvm-objcopy/Object.cpp:39
+  handleAllErrors(BufferOrErr.takeError(), [this](const ErrorInfoBase &) {
+    error("failed to open " + getName());
+  });
----------------
This should use the error info's message. Otherwise, the user won't be able to know what is wrong. I think this is probably also one that is easy to test, by specifying a location in a non-existent directory?


================
Comment at: tools/llvm-objcopy/Object.h:130
+
+class Buffer {
+  StringRef Name;
----------------
jhenderson wrote:
> alexshap wrote:
> > alexshap wrote:
> > > jhenderson wrote:
> > > > I'm probably missing something, but what's wrong with the existing buffer classes in llvm? Would it make more sense to extend them in the general case, rather than a specific reimplementation here?
> > > > 
> > > > It would be nice to see some comments describing what the interface functions do.
> > > it's not a reimplementation per se. FileOutputBuffer and WritableMemoryBuffer share a big portion of the interface but don't have a common base class. They are used in many places across  llvm - so i didn't want to change them for now. I've tried several approaches, I liked this one for several reasons: (imo) it's cleaner than the other options, the concerns are nicely factored out, the writers don't depend on the implementation of the buffers (but depend on the interface). 
> > i'd prefer to add a TODO here to refactor the buffer classes in llvm and then get rid of these, rather than blocking this patch or doing it here. 
> Okay, that's fair, I think, though I'd like @jakehehrlich's thoughts on it. I would like a comment along with the TODO explaining why we chose do it this way then.
I think you've been a bit premature in wrapping with this comment. You can just write it all on a single line, and then let clang-format wrap it appropriately.


================
Comment at: tools/llvm-objcopy/Object.h:150
+class FileBuffer : public Buffer {
+  std::unique_ptr<FileOutputBuffer> Out;
+
----------------
Out doesn't seem like a good name to me, as it is unclear. I'd call it "OutputFile" or even just "File" or "Buf", since it doesn't necessarily have to be used purely for output. Similar comment for MemBuffer's member.


================
Comment at: tools/llvm-objcopy/Object.h:160
+
+class MemBuffer : public Buffer {
+  std::unique_ptr<WritableMemoryBuffer> Out;
----------------
I think you should just call this "MemoryBuffer" if that name doesn't clash with anything already.


Repository:
  rL LLVM

https://reviews.llvm.org/D48413