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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Do you actually need `WasmConfig` and `COFFConfig`? If we can avoid adding them, that would be nice.



================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFConfig.h:9
+
+#ifndef LLVM_TOOLS_LLVM_OBJCOPY_COFFCONFIG_H
+#define LLVM_TOOLS_LLVM_OBJCOPY_COFFCONFIG_H
----------------
Need to fix the include guards. Same in the other headers, as directed by clang-tidy.


================
Comment at: llvm/tools/llvm-objcopy/ElfConfig.h:1
-//===- ELFConfig.h ----------------------------------------------*- C++ -*-===//
+//===- ElfConfig.h --------------------------------------------------------===//
 //
----------------
jhenderson wrote:
> 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.
Still need to change `ElfConfig.h` to `ELFConfig.h` in the comment on the first line of the file.


================
Comment at: llvm/tools/llvm-objcopy/MultiFormatConfig.h:1
+//===- MultiFormatConfig.h ------------------------------------------------===//
+//
----------------
Nit: missing the "C++" bit in this line of the comment.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:273
+                               const ConfigManager &ConfigMgr) {
   int FD;
 
----------------
Perhaps add a `CommonConfig &Config` local variable to this function? Would allow you to avoid repeated `getCommonConfig()` calls.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:324
   sys::fs::file_status Stat;
-  if (Config.InputFilename != "-") {
-    if (auto EC = sys::fs::status(Config.InputFilename, Stat))
-      return createFileError(Config.InputFilename, EC);
+  if (ConfigMgr.Common.InputFilename != "-") {
+    if (auto EC = sys::fs::status(ConfigMgr.Common.InputFilename, Stat))
----------------
Similar to elsewhere, consider adding a `CommonConfig &Config` local variable to reduce the repetitive `ConfigMgr.Common` statements, and reduce changes in this diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99055