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

[Seiya Nuta via Phabricator via llvm-commits]

seiya marked 4 inline comments as done.
seiya added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/only-section.test:58-59
+
+## Remove all sections without warning if the specified section name is not
+## present in the input.
+# RUN: llvm-objcopy --only-section __TEXT,__nonexistent %t %t5
----------------
jhenderson wrote:
> You say "without warning" here, but don't actually check for a warning, which you probably should do, if you're going to mention it in the comment.
Removed "without warning" from the comment since the behavior is not so surprising.


================
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()) {
----------------
jhenderson wrote:
> 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.
Now I noticed that comment does not make sense. This TODO is for LLVM code, not GNU objcopy. Rephrased the comment.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOConfig.h:22-23
+
+struct MachOCopyConfig {
+};
+
----------------
jhenderson wrote:
> 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!
>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!

Good point! Added a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65541