[PATCH] D99055: [llvm-objcopy] Refactor CopyConfig structure.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've done another skim. I should be able to give this a fuller review again tomorrow.



================
Comment at: llvm/tools/llvm-objcopy/ElfConfig.h:1
-//===- ELFConfig.h ----------------------------------------------*- C++ -*-===//
+//===- ElfConfig.h --------------------------------------------------------===//
 //
----------------
Revert the "C++" bit of this change. The old-style was correct - see https://llvm.org/docs/CodingStandards.html#file-headers. In fact, looking at the other headers in this change, please add this to them too.

Also "ELFConfig.h" not "ElfConfig.h" is more correct, as "ELF" is an accronym, so should be all-caps in general.


================
Comment at: llvm/tools/llvm-objcopy/ElfConfig.h:29-30
 
-struct ELFCopyConfig {
+// Elf specific configuration for copying/stripping a single file.
+struct ElfConfig {
   Optional<uint8_t> NewSymbolVisibility;
----------------
Similar comment: "ELF specific" and `ELFConfig` are the correct spellings.


================
Comment at: llvm/tools/llvm-objcopy/MachOConfig.h:15
+
+// MachO specific configuration for copying/stripping a single file.
+struct MachOConfig {};
----------------



================
Comment at: llvm/tools/llvm-objcopy/MultiFormatConfig.h:12-14
+#include "llvm/Support/Allocator.h"
+#include "llvm/Support/Error.h"
+#include <vector>
----------------
vector and Allocator.h look unnecessary to me.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:11
 #include "COFF/COFFObjcopy.h"
-#include "CopyConfig.h"
+#include "COFFConfig.h"
+#include "CommonConfig.h"
----------------
Is there a particular reason you haven't put these files inside the COFF/ELF/MachO/wasm subdirectories?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99055