[PATCH] D85540: [llvm-libtool-darwin] Add support for -l and -L

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/L-and-l.test:1
+## This test checks that -l and -L options work correctly.
+
----------------
sameerarora101 wrote:
> It doesn't make sense to test `-L` option individually as the directory specified under that option is used (prepended) only if some library is also specified with option `-l`. 
> 
> To test `-l` individually, I would need to create a library in one of the standard directories (`/lib/`, `/usr/lib`, `/usr/local/lib`). Is there a way I can do that in a test file? Thanks.
I don't think so. Other tools have options (like `--sysroot`) to re-root the search, but it doesn't appear that libtool has anything like that, so I don't think we'll be able to test that case.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/L-and-l.test:7
+# RUN: rm -rf %t/dirname && mkdir -p %t/dirname
+# RUN: llvm-ar cr %t/dirname/archive.o %t-input1.o %t-input2.o
+
----------------
I'd prefer for a file ending in `.o` to actually be an object file and not an archive :)


================
Comment at: llvm/test/tools/llvm-libtool-darwin/L-and-l.test:85
+
+# NOT-VALID: error: '[[FILE]]': The file was not recognized as a valid object file
----------------
Other test ideas:
- If multiple directories specified with `-L` have the same named file in them, the file from the first directory should be selected.
- You should be able to combine `-l` with input files specified normally (i.e. as paths).
- What happens if we specify the same `-l` option twice? (It's okay if it differs from cctools' behavior, but we should make a note.)


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:77
+
+static cl::list<std::string> LDirs(
+    "L",
----------------
How about `LibrarySearchDirs`?


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:84
+
+static const std::vector<StringRef> StandardDirs{
+    "/lib/",
----------------
sameerarora101 wrote:
> I defined `StandarDirs` based on cctools' libtool:
> 
> ```
> /* the standard directories to search for -lx names */
> char *standard_dirs[] = {
>     "/lib/",
>     "/usr/lib/",
>     "/usr/local/lib/",
>     NULL
> };
> ```
Nit: I'd prefer a `std::array`. I think you can make it `constexpr` too?

I'd also prefer `StandardSearchDirs`.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:85-87
+    "/lib/",
+    "/usr/lib/",
+    "/usr/local/lib/",
----------------
I don't think you need the trailing slashes.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:99
+    sys::path::append(Path, Dir, FileName);
+    StringRef PathString = Path;
+
----------------
Any reason for creating this variable? SmallString can be implicitly converted to StringRef instead, and it can also be converted to `std::string` explicitly (e.g. via `std::string(Path)`).


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:101
+
+    if (sys::fs::exists(PathString)) {
+      return PathString.str();
----------------
Nit: you don't need the braces


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:108
+  for (StringRef Dir : StandardDirs) {
+    sys::path::append(Path, Dir, FileName);
+    StringRef PathString = Path;
----------------
The duplication between the two loops is unfortunate, but I don't think the body of the loops is large enough to justify moving it out into a function (especially if you get rid of the `PathString` variable). What we really want is a C++ equivalent to Python's `itertools.chain`, and I think we might get that with ranges in C++20.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:122
+static Error processCommandLineLibraries() {
+  for (std::string s : Libraries) {
+    StringRef BaseName = StringRef(s);
----------------
`const &`


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:123
+  for (std::string s : Libraries) {
+    StringRef BaseName = StringRef(s);
+
----------------
One of the following should work, I think.

```
StringRef BaseName = s;
StringRef BaseName(s);
```


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:127
+    if (BaseName.endswith(".o"))
+      FullBaseName = BaseName.str();
+    else
----------------
I would construct a `Twine` and have `searchForFile` take `const Twine &` instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85540