[PATCH] D65541: [llvm-objcopy][MachO] Implement --only-section

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOConfig.cpp:22-23
+  //
+  // TODO: Support section renaming in GNU objcopy for compatibility (see
+  // http://lists.llvm.org/pipermail/llvm-dev/2019-May/132570.html).
+  if (!Config.OnlySection.empty()) {
----------------
I know this was there before, but I'm not sure I understand this TODO. I think it's saying that support should be added to GNU objcopy for something? If so, I don't think you want this TODO in the code. TODOs in LLVM code should be about LLVM changes that should be made at some point.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOConfig.h:12-13
+
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
----------------
I don't think you need these two includes here.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOConfig.h:22-23
+
+struct MachOCopyConfig {
+};
+
----------------
Is this clang-formatted? I thought it should be `struct MachOCopyConfig {};` but could be wrong easily.

It might be worth a simple comment saying why there's an empty struct. I initially thought it was an incorrect attempt to forward declare it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65541