[PATCH] D47052: [llvm-objcopy] Fix the behavior of --strip-* and --keep-symbol

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.

LGTM, with a few nits.



================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:227
+  // TODO: update or remove symbols only if
+  // there is an option that effects them.
+  if (Obj.SymbolTable) {
----------------
Nit: effects -> affects

Also, I think you've wrapped unnecessarily. Clang-format doesn't "undo" line breaks in comments to match up to the column width.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:381-384
+  // If the option --keep-symbol has been specified
+  // and at least one of those symbols is present 
+  // (equivalently, the updated symbols table is not empty)
+  // the symbols table and the strings table should not be removed.
----------------
See my earlier comments re. comment wrapping.

symbols table -> symbol table (x2)
strings table -> string table


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:386
+  if (!Config.SymbolsToKeep.empty() && !Obj.SymbolTable->empty()) {
+    RemovePred = [&Obj, Config, RemovePred](const SectionBase &Sec) {
+      if (&Sec == Obj.SymbolTable || &Sec == Obj.SymbolTable->getStrTab())
----------------
I don't think you need Config in the capture list?


Repository:
  rL LLVM

https://reviews.llvm.org/D47052