[PATCH] D60042: [llvm-objcopy] Add --prefix-alloc-sections

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

Just a couple small comments, but also LGTM



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/prefix-alloc-sections.test:59
+# CHECK:   Section (4) .rel.alloc_prefix.text {
+# CHECK:   }
+# CHECK:   Section (5) .rela.alloc_prefix.data {
----------------
Checking the `}` isn't necessary (unless you're checking a whole section w/ CHECK-NEXT)


================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp:179-181
+      !Config.SymbolsPrefix.empty() || !Config.AllocSectionsPrefix.empty() ||
+      !Config.AddSection.empty() ||
       !Config.DumpSection.empty() || !Config.KeepSection.empty() ||
----------------
I think this block needs to be clang formatted


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:650-654
+          if (TargetSec->Name.find(Config.AllocSectionsPrefix) == 0)
+            Sec.Name = (prefix + TargetSec->Name).str();
+          else
+            Sec.Name =
+                (prefix + Config.AllocSectionsPrefix + TargetSec->Name).str();
----------------
Can you add a comment why this block is necessary?

It looks like all the test coverage is .rel sections coming after their target section. Is it possible to add a test case for the other half of this clause? e.g. a section w/ .rel.data coming before .data?


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

https://reviews.llvm.org/D60042