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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

> Okay, maybe here's my best, hopefully uncontroversial, suggestion, since it would be good to get this in for the LLVM 7.0 cut today(?): leave them in but mark them as "legacy" or for GNU compatibility e.g: `Flags supported for GNU compatibility: alloc, load...` in both error message and help text. If and when we add more flags, for a better/clearer UI, we can make that a separate sentence like `Supported Flags: alloc, write, execinstr,... Additional flags supported for GNU compatibility: load, noload...` etc.

That wording sounds good to me. I've updated the errors/tests.

> LGTM with those changes.

Thanks for the thorough review!



================
Comment at: tools/llvm-objcopy/ObjcopyOpts.td:32-35
+                      HelpText<"Renames a section from old to new, optionally "
+		               "with specified flags. Supported flags: alloc, "
+			       "load, noload, readonly, debug, code, data, "
+			       "rom, share, contents, merge, strings.">;
----------------
jhenderson wrote:
> jhenderson wrote:
> > Is this clang-formatted? It looks weird...
> I just realised that this is in the .td file, so clang-format might not be applicable, but it looks better now anyway, thanks!
Actually, I did use clang-format (forcing it with "git-clang-format --extensions td master") to fix my ugly manual formatting. It's similar enough to C++ that clang-format //sometimes// works well on small diff regions, although it doesn't seem like it's 100% ready for full-file formatting.


Repository:
  rL LLVM

https://reviews.llvm.org/D49870