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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

> The functions in the library header need doxygen style comments to explain the interface. Possibly these could be added in a follow-up patch.

Ok.

> I think if we are moving files around, now is a good time to run a blanket clang-format on the files. Whether that should be done as part of the patch or a separate follow-on one, I don't know.

Ok, but I think it is better to do in a separate patch.

> Probably not this patch, but we should consider our testing strategy for the libray and llvm-objcopy going forward. Some examples for this dicussion: should we move the existing lit tests? Should we port/duplicate some of them as gtest unit tests? How should new library features be tested (gtest or lit)? How should new llvm-strip/objcopy/... options be tested (bearing in mind in theory there should be library testing etc etc)? I think this sort of discussion probably belongs on the mailing list.

OK, I will start that thread soon.



================
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 {
----------------
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.


================
Comment at: llvm/include/llvm/ObjCopy/CopyConfig.h:126-127
 
 // Matcher that checks symbol or section names against the command line flags
 // provided for that option.
 class NameMatcher {
----------------
jhenderson wrote:
> This comment probably needs updating to better match the new usage - but see out-of-line comment.
ok.


================
Comment at: llvm/include/llvm/ObjCopy/ObjCopy.h:19-22
+/// The function applies transformation described by Config to
+/// the specified binary and writes result into the Out. It does
+/// the dispatch based on the format of the input binary
+/// (ELF, MachO or COFF).
----------------
jhenderson wrote:
> I think details of the output format shouldn't be described in the comment - theoretically objcopy could even mutate from one object format to another (see e.g. the IHEX stuff).
ok.


================
Comment at: llvm/lib/ObjCopy/COFF/Object.cpp:1
 //===- Object.cpp ---------------------------------------------------------===//
 //
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88827