[PATCH] D69146: [install-name-tool] Add first bits for install-name-tool
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Mar 22 00:30:16 PDT 2020
MaskRay added inline comments.
================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:320-322
+ const bool IsStrip = Stem.contains("strip");
+ const bool IsInstallNameTool = (Stem.contains("install-name-tool") ||
+ Stem.contains("install_name_tool"));
----------------
rupprecht wrote:
> Once we've gone past 2 tools, I think we should have an enum instead -- and maybe add something to `StringSwitch` so you could write:
>
> ```
> ToolType Tool = StringSwitch(Stem)
> .Contains("strip", ToolType::Strip)
> .Contains("install-name-tool", ToolType::InstallNameTool)
> .Contains("install_name_tool", ToolType::InstallNameTool)
> .Default(ToolType::Objcopy);
> ```
It seems many comments are not addressed.
This one regressed D54193 (llvm-strip-9 is not recognized). It was my problem that I did not add a test because I did not know I could use a trick like D71301 to make testing work on at least non-Windows OSes.
Please check D76562
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69146/new/
https://reviews.llvm.org/D69146
More information about the llvm-commits
mailing list