[PATCH] D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:121
 
+struct SectionRenameConfig {
+  StringRef NewName;
----------------
maybe drop Config from the name ? doesn't seem to provide too much value, although i don't insist


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:196
 
+enum SectionFlag {
+  SecNone = 0,
----------------
I think in any case it should not be in the global namespace,
but more importantly, probably it would be better to use ELF* constants directly rather than introducing this enum.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:655
+    SectionConfig.NewName = NameAndFlags[0];
+
+    if (NameAndFlags.size() > 1) {
----------------
above:
i think now things are getting a bit more complicated and this code deserves to be moved into a helper function (it would parse the passed StringRef and return SectionConfig / Error)


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:658
+      SectionFlag Flags = SectionFlag::SecNone;
+      for (int i = 1; i < (int)NameAndFlags.size(); ++i) {
+        SectionFlag Flag = ParseSectionName(NameAndFlags[i]);
----------------
size_t + no C-style casts, please


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:679
+    if (!Config.SectionsToRename
+             .try_emplace(Old2New.first, std::move(SectionConfig))
+             .second)
----------------
std::move is unnecessary here - that struct essentially contains only integral types (pointers / numbers) - it doesn't have a move-ctor, thus std::move will only introduce an unnecessary cast here.


Repository:
  rL LLVM

https://reviews.llvm.org/D49870