[PATCH] D88827: [llvm-objcopy][NFC] Move core implementation of llvm-objcopy into separate library.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/include/llvm/ObjCopy/Buffer.h:23-24
 // abstract interface and doesn't depend on a particular implementation.
 // TODO: refactor the buffer classes in LLVM to enable us to use them here
 // directly.
 class Buffer {
----------------
avl wrote:
> jhenderson wrote:
> > Maybe as part of a separate patch, it would be worth taking a look at this TODO. It would be great if the Buffer could be removed from the library API and generic LLVM buffers used instead (for example an in-memory buffer or a file buffer, depending on what people want to do).
> agreed, but I think it is better to do in separate patch.
@alexshap @jhenderson 

Speaking of class Buffer refactoring. I do not think that this comment is fully valid:


```
// The class Buffer abstracts out the common interface of FileOutputBuffer and
// WritableMemoryBuffer so that the hierarchy of Writers depends on this
// abstract interface and doesn't depend on a particular implementation.
// TODO: refactor the buffer classes in LLVM to enable us to use them here
// directly.
```

It suggests to create some common interface for FileOutputBuffer and WritableMemoryBuffer.
Which is assumed to look similar to this: 


```
class Buffer {
  StringRef Name;

public:
  virtual ~Buffer();
  virtual Error allocate(size_t Size) = 0;
  virtual uint8_t *getBufferStart() = 0;
  virtual Error commit() = 0;

  explicit Buffer(StringRef Name) : Name(Name) {}
  StringRef getName() const { return Name; }
};

```
There exists a problem with methods commit() and allocate(). commit() is a redundant for WritableMemoryBuffer.
adding it to WritableMemoryBuffer would require to patch all current usages of WritableMemoryBuffer.
So it looks incorrectly to use it for common parent interface of FileOutputBuffer and WritableMemoryBuffer.
allocate() suggests another way of buffer creation. Currently, buffers are created by static creation methods:  


```
static std::unique_ptr<WritableMemoryBuffer>
getNewMemBuffer(size_t Size, const Twine &BufferName = "");

static Expected<std::unique_ptr<FileOutputBuffer>>
create(StringRef FilePath, size_t Size, unsigned Flags = 0);
```

Adding "virtual Error allocate(size_t Size)" would lead to creation empty buffer by static creation method 
and then call to allocate(). This does not seem a good addition to the already existed FileOutputBuffer and WritableMemoryBuffer. FileOutputBuffer and WritableMemoryBuffer assume another use cases than Buffer.

Actually, what is neccessary by objcopy is method:


```
uint8_t *createBuffer ( size_t Size ); 
```

All other functionality is redundant and could be removed from objcopy.

what do you think about following design?


```
LazyBuffer {
  StringRef Name;

  virtual StringRef getName() const { return Name; }
  virtual uint8_t *createBuffer ( size_t Size ) = 0;
};

MemoryLazyBuffer : public LazyBuffer {
  virtual uint8_t *createBuffer ( size_t Size ) {
    Buffer = WritableMemoryBuffer::getNewMemBuffer(Size, Name);
    return Buffer->getBufferStart();
  }

  std::unique_ptr<WritableMemoryBuffer> Buffer;
};

FileLazyBuffer : public LazyBuffer {
  virtual uint8_t *createBuffer ( size_t Size ) {
    Buffer = FileOutputBuffer::create(Name, Size);
    return Buffer->getBufferStart();
  }

  std::unique_ptr<FileOutputBuffer> Buffer;
};
```

Usage:


```
static Error executeObjcopyOnBinary(CopyConfig &Config, object::Binary &In,
                                    LazyBuffer &Out);


MemoryLazyBuffer MB("name");
executeObjcopyOnBinary(Config, Input, MB);

FileLazyBuffer MB("name");
executeObjcopyOnBinary(Config, Input, MB);
if (MB.Buffer)
  MB.Buffer->commit();

```


Another alternative is that library always writes to general MemoryBuffer :

static Expected<std::unique_ptr<MemoryBuffer>> executeObjcopyOnBinary(CopyConfig &Config, object::Binary &In);

and later this MemoryBuffer would be written into the file by llvm-objcopy.cpp.

What do you think?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88827