[PATCH] D41100: [llvm-objcopy] Refactor llvm-objcopy to allow llvm-strip to differ

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/basic-strip.test:1
 # RUN: yaml2obj %s > %t
 # RUN: llvm-strip %t %t2
----------------
I think you accidentally made this a diff against your previous version, although it's a new file!


================
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) !=
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > Why this change?
> ToRemove (and Keep and OnlyKeep) are members of this. This function became a method so that I could change all the variables in it via CopyConfig's members. Adding "this" to the capture list lets me use ToRemove in this function.
At least in this particular case, you are already capturing everything by reference anyway, so ToRemove seems to be available (at least in my MSVC build). The other two cases are fine. I'd also support here replacing the general & capture with an explicit this capture, since only ToRemove is used.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:326
+template <class ELFT>
+void CommonConfig(CopyConfig<ELFT>& Config) {
+  if (OutputFilename.empty())
----------------
It feels to me like this should be a member of the CopyConfig class. Maybe even part of the constructor. Is there ever a case you can imagine when you would NOT want to call CommonConfig?


Repository:
  rL LLVM

https://reviews.llvm.org/D41100