Add support for -filelist option for llvm-libtool-darwin
-filelist option allows for passing in file containing a list of filenames.
Depends on D83520.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Documentation for the new option?
llvm/test/tools/llvm-libtool-darwin/filelist.test | ||
---|---|---|
29 | Why are you trying to delete some files here? | |
33 | I'm not sure this comment is accurate for what the error is indicating. It needs to provide more context. | |
67 | This seems like essentially a duplicate of the earlier error case? | |
95 | Additional possible test-cases:
| |
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
47 | Nit: your -o option doesn't include a trailing full stop in its description. Be consistent. |
llvm/test/tools/llvm-libtool-darwin/filelist.test | ||
---|---|---|
29 | I just wanted to make sure that %basename_t.tmp-input1.o didn't exist in the current directory. However, I have now removed the error test just below (and therefore removed RUN: rm -f %t-input1.o && rm -f %basename_t.tmp-input1.o) as it was similar to the "file not in filelist test" as you highlighted. Thanks! | |
67 | yup, you are right. I have now removed the previous test case | |
95 | 3rd case is already there: ## Check that error is thrown when file in filelist doesn't exist (with dir): # RUN: rm -rf %t/Invalid-Dir && mkdir -p %t/Invalid-Dir # RUN: echo 'no-such-file' > %t.invalid-list.txt # RUN: not llvm-libtool-darwin -static -o %t.lib -filelist %t.invalid-list.txt,%t/Invalid-Dir 2>&1 | \ # RUN: FileCheck %s --check-prefix=FILE-ERROR -DFILE=%t/Invalid-Dir/no-such-file I have modified the comment to make it more clear. Added the first 2 cases now. Thanks. |
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
---|---|---|
69–70 | I was getting an assertion error that filenames can't be empty. Hence, now I am explicitly throwing the error here only. |
llvm/docs/CommandGuide/llvm-libtool-darwin.rst | ||
---|---|---|
52 | I'd suggest "Whitespace on a line is assumed..." | |
llvm/test/tools/llvm-libtool-darwin/filelist-error-blank-lines.test | ||
9 ↗ | (On Diff #279573) | Could you just use echo '' >> %t.blank-line.txt to get your blank line, so that this can run on Windows? |
15 ↗ | (On Diff #279573) | Similarly, here, use echo -n " " here. Why the -n at all in fact in this case? |
17 ↗ | (On Diff #279573) | I'd suggest using FileCheck --strict-whitespace in this case to be able to match the filename properly. |
llvm/test/tools/llvm-libtool-darwin/filelist.test | ||
60 | error -> an error | |
67 | an error | |
68 | I'd recommend a different name for the directory. The directory itself is not invalid, so calling it that is a little confusing. How about just reusing the directory from before? | |
75 | an error | |
95 | Thanks. What about one for the directory not existing at all? | |
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
69–70 | I'd be tempted to use a different error message to show that this error isn't a system error. Perhaps simply "filename cannot be empty". |
LGTM, with nit, thanks!
llvm/test/tools/llvm-libtool-darwin/filelist.test | ||
---|---|---|
96 | Let's check the full error line. Presumably, this at least has error: file list file: or something similar before it? |
LGTM
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
---|---|---|
78 | Nit: SmallString has an operator std::string, so you should be able to convert directly without the intermediate Twine. I'm not sure if just InputFiles.push_back(Path) will compile, but a cast should definitely trigger the conversion, e.g. InputFiles.push_back(static_cast<std::string>(Path)). |
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
---|---|---|
78 | Yup, casting works. Thanks. |
I'd suggest "Whitespace on a line is assumed..."