[PATCH] D111488: [Clang][clang-nvlink-wrapper] Pass nvlink path to the wrapper

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur requested changes to this revision.
Meinersbur added a comment.
This revision now requires changes to proceed.

Thanks for the path, but command line parsing should be done properly.



================
Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:50
+static cl::opt<std::string>
+    NvlinkUserPath("path", cl::desc("path of directory containing nvlink"),
+                   cl::cat(ClangNvlinkWrapperCategory));
----------------
I suggest a more descriptive flag, `path` is too general, like `--nvlink` or `--nvlink-command` (bugpoint uses `--opt-command` for the path to `opt`)


================
Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:124
   sys::PrintStackTraceOnErrorSignal(argv[0]);
-
   if (Help) {
----------------
[nit] unrelated whitespace change


================
Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:133
   sys::PrintStackTraceOnErrorSignal(argv[0]);
-
   if (Help) {
     cl::PrintHelpMessage();
----------------
`cl::ParseCommandLineOptions(argc, argv,"Program description")` must have been called beforehand to make this work.


================
Comment at: clang/tools/clang-nvlink-wrapper/ClangNvlinkWrapper.cpp:154
+    StringRef ArgRef(Arg);
+    auto NvlPath = ArgRef.startswith_insensitive("--path=");
     if (sys::path::extension(Arg) == ".a") {
----------------
This way of parsing does not use the declared `NvlinkUserPath` option. Use `cl::ParseCommandLineOptions` do parse the arguments and 
```
cl::list<std::string> InputFiles(cl::Positional, cl::desc("<input files>..."));
```
to catch all non-option arguments.

The way it is done here also does not catch the `--path <nvlink>` (without equal sign) syntax.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111488