[llvm-libtool-darwin] Add support for -filelist option

[sameerarora101]

Summary

Add support for -filelist option for llvm-libtool-darwin
-filelist option allows for passing in file containing a list of filenames.
Depends on D83520.

[jhenderson]

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:

1. Multiple file lists specified.
2. File is in working directory, but a directory name is specified.
3. Directory exists but does not contain the requested file.

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.

[jhenderson]

By the way, the pre-merge bot is saying your new test is failing on Windows.

[sameerarora101]

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.

[sameerarora101]

Add documentation and updating test file

[sameerarora101]

Unsupport a test on windows and thow blank-line error earlier.

[sameerarora101]

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.

[MaskRay]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

61

line_iterator supports is_at_eof. You can delete E

184

Prefer return to exit in main

[jhenderson]

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
file -> a file
filelist -> the filelist
cwd -> the cwd

67

an error
the directory

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
a file
the cwd

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".

[sameerarora101]

llvm/test/tools/llvm-libtool-darwin/filelist-error-blank-lines.test

9 ↗

(On Diff #279573)

oh nice, thanks!

15 ↗

(On Diff #279573)

Yup, works without -n.

llvm/test/tools/llvm-libtool-darwin/filelist.test

95

Added now.

[sameerarora101]

• Supporting test on windows
• Using is_at_eof for line_iterator

[jhenderson]

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?

[sameerarora101]

Updating the error message.

[smeenai]

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)).

[sameerarora101]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

78

Yup, casting works. Thanks.

[sameerarora101]

Twine(Path).str() -> static_cast<std::string>(Path)