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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/Object.h:130
+
+class Buffer {
+  StringRef Name;
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D48413