[PATCH] D69146: [install-name-tool] Add first bits for install-name-tool

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In addition to the missing test cases I've highlighted, you might want to add a test case for response file usage too, in the same way as llvm-strip and llvm-objcopy are tested.



================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-add-rpath.test:1
+# RUN: yaml2obj %p/Inputs/i386.yaml > %t.i386
+# RUN: llvm-install-name-tool -add_rpath @executable_path/. %t.i386
----------------
A brief comment at the top of the file summarising the purpose of the test would be helpful. A number of newer binutils tests have them.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-add-rpath.test:5
+
+# NEW_RPATH: cmd LC_RPATH
+# NEW_RPATH-NEXT: cmdsize
----------------
NEW_RPATH -> NEW-RPATH maybe? Similar comment below for DUPLICATE_RPATH.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-add-rpath.test:12-14
+# RUN: yaml2obj %p/Inputs/x86_64.yaml > %t.x86_64
+# RUN: llvm-install-name-tool -add_rpath @executable_path/. %t.x86_64
+# RUN: llvm-objdump -p %t.x86_64 | FileCheck --check-prefix=NEW_RPATH %s
----------------
I think it would be marginally easier to follow the test if you reflowed it as:


```
# RUN: yaml2obj i386
# RUN: llvm-install-name-tool i386
# RUN: llvm-objdump

# RUN: yaml2obj x86_64
# RUN: llvm-install-name-tool x86_64
# RUN: llvm-objdump

# NEW_RPATH:
...

# RUN: not llvm-install-name-tool DUPLICATE

# DUPLICATE_RPATH:

# RUN: not llvm-install-name-tool NO_INPUT

# NO_INPUT:
```


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:715-717
+// ParseInstallNameToolOptions returns the config and sets the input arguments.
+// If a help flag is set then ParseInstallNameToolOptions will print the help
+// messege and exit.
----------------
Do we need this comment both in the header and in the .cpp?


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:727
+
+  if (InputArgs.size() == 0) {
+    printHelp(T, errs(), "llvm-install-name-tool");
----------------
Test case?


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:732-734
+  if (InputArgs.hasArg(INSTALL_NAME_TOOL_help)) {
+    printHelp(T, outs(), "llvm-install-name-tool");
+    exit(0);
----------------
Test case?


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:737-741
+  if (InputArgs.hasArg(INSTALL_NAME_TOOL_version)) {
+    outs() << "llvm-install-name-tool, compatible with cctools "
+              "install_name_tool\n";
+    cl::PrintVersionMessage();
+    exit(0);
----------------
Test case?


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:749
+  for (auto Arg : InputArgs.filtered(INSTALL_NAME_TOOL_UNKNOWN))
+    return createStringError(errc::invalid_argument, "unknown argument '%s'",
+                             Arg->getAsString(InputArgs).c_str());
----------------
Test case?


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:757
+    return createStringError(errc::invalid_argument,
+                             "llvm-install-name-tool expects a single input file");
+  Config.InputFilename = Positional[0];
----------------
clang-format?
Test case?


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.h:221-222
 
+// ParseInstallNameToolOptions returns the config and sets the input arguments. If a
+// help flag is set then ParseInstallNameToolOptions will print the help messege and
+// exit.
----------------
Line length here looks too long. Clang-format?


================
Comment at: llvm/tools/llvm-objcopy/MachO/Object.cpp:13
 
+void Object::addLoadCommand(LoadCommand LC) { LoadCommands.push_back(std::move(LC)); }
+
----------------
clang-format?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69146