[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