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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added subscribers: mtrent, echristo, dblaikie.
alexshap added a comment.

I have some general comments / concerns (in addition to the inline comment).
The interface of the library is important and once it's committed and people start using the library in multiple places it might be harder to make changes / fix issues
(unfortunately this has already happened in LLVM a few times in the past) .

1. CopyConfig as a part of the interface in its current form seems to be not "in a perfect shape". E.g. some ELF-specific options are inside the struct ELF, some others are just regular fields.

Some fields make sense in the context of the tool but they don't (at least at quick glance) in the context of a library function.

2. class Buffer - I agree with @jhenderson's comment. Just want to note that in general there are multiple ways how to address it, but this requires some thinking.

1, 2 - perhaps, this can be refactored / reorganized / better documented, but it would be great to have it done before the introduction of the library.

I'd like to point out that not all the operations supported by llvm-objcopy can be performed in-memory (e.g. splitting dwo will create a new file). If a library function has such side effects (creates new objects on the local file system) it would be good to have it documented.

cc: @mtrent , @dblaikie, @echristo



================
Comment at: llvm/lib/ObjCopy/COFF/Object.cpp:1
 //===- Object.cpp ---------------------------------------------------------===//
 //
----------------
avl wrote:
> jhenderson wrote:
> > Whilst you're moving this and the equivalent files for other formats around, could you please rename them to be obvious from the filename which format they are for (same goes for their headers), please? For example, COFF/Object.cpp -> COFF/COFFObject.cpp. The reason for this is that when using the Visual Studio IDE, all the "Object.cpp" files end up listed next to each other in the file browser, and the only way of figuring out which is which is by opening them and seeing.
> ok.
@jhenderson, I'm sorry to disagree, but renaming files this way doesn't seem to be a good idea and the provided justification doesn't appear to be sufficient.
Since this file contains the implementation of what's declared in Object.h I would strongly prefer to have it named Object.cpp given it is already located in the corresponding folder. Visual Studio IDE might have some peculiarities but having consistent naming is important, adding such prefixes doesn't seem to be a good approach. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88827