[PATCH] D65991: [llvm-objcopy] Move duplicate tablegen from objcopy and strip into one file

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/CommonAliases.td:3
+//===----------------------------------------------------------------------===//
+// Aliases must be defined after what they alias. Initially, I had these
+// aliases in CommonOpts.td, but putting them there causes CommonOpts.td:24:15:
----------------
I bet that if you adopt my changes to not use the classes below, this is unnecessary.


================
Comment at: llvm/tools/llvm-objcopy/CommonOpts.td:15
     : Flag<["--"], "allow-broken-links">,
-      HelpText<"Allow llvm-strip to remove sections even if it would leave "
+      HelpText<"Allow "#tool#" to remove sections even if it would leave "
                "invalid section references. The appropriate sh_link fields "
----------------
You can avoid the need for the class here by rewording slightly to say "Allow the tool to..."


================
Comment at: llvm/tools/llvm-objcopy/CommonOpts.td:21
     : Flag<["--"], "enable-deterministic-archives">,
-      HelpText<"Enable deterministic mode when stripping archives (use zero "
+      HelpText<"Enable deterministic mode when "#action#" archives (use zero "
                "for UIDs, GIDs, and timestamps).">;
----------------
Here and in the disable case, you can avoid the class by rewording slightly to something like "when operating on archives".


================
Comment at: llvm/tools/llvm-objcopy/CommonOpts.td:41
+    : Flag<["--"], "strip-all-gnu">,
+      HelpText<"Compatible with GNU "#tool#"'s --strip-all">;
+
----------------
Again, I think you can remove the reference to the tool name entirely here i.e. "GNU's --strip-all".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65991