[PATCH] D60324: [llvm-objcopy] Add switch to allow removing referenced sections

[James Henderson via Phabricator via llvm-commits]

jhenderson marked 2 inline comments as done.
jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:440
+          "referenced by the symbol table %s",
+          SymbolNames->Name.data(), this->Name.data());
   return removeSymbols(
----------------
grimar wrote:
> Perhaps also be consistent here?
> 
> 
> ```
> if (ToRemove(SymbolNames)) {
>     if (!AllowBrokenDependencies)
>       return createStringError(
>           llvm::errc::invalid_argument,
>           "String table %s cannot be removed because it is "
>           "referenced by the symbol table %s",
>           SymbolNames->Name.data(), this->Name.data());
> 
>     SymbolNames = nullptr;
> }
> ```
Thanks, I missed this one.


================
Comment at: tools/llvm-objcopy/ObjcopyOpts.td:13
+def allow_broken_dependencies
+    : Flag<["-", "--"], "allow-broken-dependencies">,
+      HelpText<"Allow llvm-objcopy to remove sections even if it would leave "
----------------
grimar wrote:
> Not strong opinion here, but `allow-broken-dependencies` looks like a bit too long option name.
> Will `allow-broken-deps`/`allow-broken-links` sound better?
I don't like abbreviations in switches, because I don't know what "deps" stands for. However, `allow-broken-links` is reasonable. I was thinking this switch could also be used for other relationships via sh_info, but I don't know of any yet where this is needed.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60324