[PATCH] D84206: [llvm-libtool-darwin] Add support for -filelist option

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-libtool-darwin.rst:52
+ Read input file names from `<listfile>`. File names are specified in `<listfile>`
+ one per line, separated only by newlines. Tabs and spaces on a line are assumed
+ to be part of the filename. If the directory name, `dirname`, is also
----------------
I'd suggest "Whitespace on a line is assumed..."


================
Comment at: llvm/test/tools/llvm-libtool-darwin/filelist-error-blank-lines.test:9
+# RUN: echo %t-input2.o > %t.blank-line.txt
+# RUN: echo -ne "\n"   >> %t.blank-line.txt
+# RUN: not llvm-libtool-darwin -static -o %t.lib -filelist %t.blank-line.txt 2>&1 | \
----------------
Could you just use `echo '' >> %t.blank-line.txt` to get your blank line, so that this can run on Windows?


================
Comment at: llvm/test/tools/llvm-libtool-darwin/filelist-error-blank-lines.test:15
+# RUN: echo %t-input2.o    > %t.space-line.txt
+# RUN: echo -ne "     \n" >> %t.space-line.txt
+# RUN: not llvm-libtool-darwin -static -o %t.lib -filelist %t.space-line.txt 2>&1 | \
----------------
Similarly, here, use `echo -n "      "` here. Why the `-n` at all in fact in this case?


================
Comment at: llvm/test/tools/llvm-libtool-darwin/filelist-error-blank-lines.test:17
+# RUN: not llvm-libtool-darwin -static -o %t.lib -filelist %t.space-line.txt 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=FILE-ERROR -DFILE=' '
+
----------------
I'd suggest using FileCheck --strict-whitespace in this case to be able to match the filename properly.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/filelist.test:59
+
+## Check that error is thrown when file in filelist doesn't exist in cwd and no dirname is specified:
+# RUN: echo 'no-such-file' > %t.invalid-list.txt
----------------
error -> an error
file -> a file
filelist -> the filelist
cwd -> the cwd


================
Comment at: llvm/test/tools/llvm-libtool-darwin/filelist.test:66
+
+## Check that error is thrown when directory exists but does not contain the requested file:
+# RUN: rm -rf %t/Invalid-Dir && mkdir -p %t/Invalid-Dir
----------------
an error
the directory


================
Comment at: llvm/test/tools/llvm-libtool-darwin/filelist.test:67
+## Check that error is thrown when directory exists but does not contain the requested file:
+# RUN: rm -rf %t/Invalid-Dir && mkdir -p %t/Invalid-Dir
+# RUN: echo 'no-such-file' > %t.invalid-list.txt
----------------
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?


================
Comment at: llvm/test/tools/llvm-libtool-darwin/filelist.test:74
+
+## Check that error is thrown when file is in cwd but dirname is specified:
+# RUN: yaml2obj %S/Inputs/input2.yaml -o %basename_t.tmp-input2.o
----------------
an error
a file
the cwd


================
Comment at: llvm/test/tools/llvm-libtool-darwin/filelist.test:94
+# RUN: not llvm-libtool-darwin -static -o %t.lib -filelist %t.space-line.txt 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=FILE-ERROR -DFILE=' '
----------------
sameerarora101 wrote:
> jhenderson wrote:
> > 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.
> 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.
Thanks. What about one for the directory not existing at all?


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:69-70
+    if (Line.empty())
+      return createStringError(std::errc::invalid_argument,
+                               "'': No such file or directory");
+
----------------
sameerarora101 wrote:
> I was getting an assertion error that filenames can't be empty. Hence, now I am explicitly throwing the error here only.
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".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84206