[PATCH] D41100: [llvm-objcopy] Add alias and "driver" for llvm-strip

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

It looks to me like you should split this change down into multiple parts, in no particular order:

1. Create a symlink for llvm-strip, and show that you can test it for a basic case.
2. Refactor the command-line options.
3. Add explicit -o option.
4. Add -S option.
5. Add support for explicit -O elf (by the way, you need a test for this, probably a simple copy, and binary diff against it not being specified).

What do you think?



================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:165
   if (!ToRemove.empty()) {
-    RemovePred = [&](const SectionBase &Sec) {
+    RemovePred = [&, this](const SectionBase &Sec) {
       return std::find(std::begin(ToRemove), std::end(ToRemove), Sec.Name) !=
----------------
Why this change?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:235
   if (!OnlyKeep.empty()) {
-    RemovePred = [RemovePred, &Obj](const SectionBase &Sec) {
+    RemovePred = [RemovePred, &Obj, this](const SectionBase &Sec) {
       // Explicitly keep these sections regardless of previous removes.
----------------
Again why this change?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:259
   if (!Keep.empty()) {
-    RemovePred = [RemovePred](const SectionBase &Sec) {
+    RemovePred = [RemovePred, this](const SectionBase &Sec) {
       // Explicitly keep these sections regardless of previous removes.
----------------
Ditto


Repository:
  rL LLVM

https://reviews.llvm.org/D41100