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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht marked 3 inline comments as done.
rupprecht added a comment.

Thanks for the review! I'm clearly still ramping up on this codebase :)



================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:196
 
+enum SectionFlag {
+  SecNone = 0,
----------------
alexshap wrote:
> 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.
> I think in any case it should not be in the global namespace,
Moved to the anonymous namespace above (w/ config classes)

> but more importantly, probably it would be better to use ELF* constants directly rather than introducing this enum.

This isn't meant to replace the ELF SHF_* constants, it's meant to map to allowed command-line flags.
For instance, "readonly" maps to "unset SHF_WRITE"


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:655
+    SectionConfig.NewName = NameAndFlags[0];
+
+    if (NameAndFlags.size() > 1) {
----------------
alexshap wrote:
> 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)
> and return SectionConfig / Error

Done -- although it felt more natural to make it return SectionConfig and just call error() directly if anything happens. Is that OK or would you prefer I return Expected<SectionConfig> and do the error() call up a level in here?


Repository:
  rL LLVM

https://reviews.llvm.org/D49870