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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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-object.o %t-input1.o %t-input2.o
+
----------------
You should place lines like this that are tied to an individual test-case, below the comment that goes with the case, as that explains the purpose of the line.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/L-and-l.test:26
+
+## Check that the library is recognosed when prepended with 'lib' and appended with '.a':
+# RUN: llvm-libtool-darwin -static -o %t.lib -lsingle-archive -L%t/dirname
----------------



================
Comment at: llvm/test/tools/llvm-libtool-darwin/L-and-l.test:116
+## Check that an error is thrown when the input library cannot be found:
+# RUN: not llvm-libtool-darwin -static -o %t.lib -larchive-object.o 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=NOT-FOUND -DFILE=archive-object.o
----------------
I might suggest changing the `-l` file's name here to something even more arbitrary such as "-lfile-will-not-exist". Since libtool searches certain directories by default, there's a (small) chance the file being searched for is in one of those directories. The weirder we make the name, the smaller this chance, and therefore the less likely this test case will fail on somebody's system.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:72-74
+        "l<x> searches for the library libx.a in the library search path. If"
+        " the string 'x' ends with '.o', then the library 'x' is searched for"
+        " without prepending 'lib' or appending '.a'"),
----------------
I just want to make sure the behaviour here is definitely what's documented and that it doesn't search for `libfoo.o.a` for `-lfoo.o`? We probably need a test case for that.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:97
+static Expected<std::string> searchForFile(StringRef FileName) {
+  SmallString<128> Path;
+
----------------
Could this be moved into the loop body, to avoid the `Path.clear()` line?


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:116
+  return createStringError(std::errc::invalid_argument,
+                           "cannot locate file for '%s'",
+                           FileName.str().c_str());
----------------



================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:121
+static Error processCommandLineLibraries() {
+  for (const std::string &s : Libraries) {
+    StringRef BaseName(s);
----------------
Can't you just do `for(StringRef BaseName : Libraries)`?

If not, at least do s -> S


================
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;
----------------
smeenai wrote:
> 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.
Perhaps a lambda could work okay here:

```
auto FindLib = [](ArrayRef<StringRef> SearchDirs) {
  for (StringRef Dir : SearchDirs) {
    SmallString<128> Path;
    sys::path::append(Path, Dir, FileName);

    if (sys::fs::exists(Path))
      return std::string(Path);
  }
  return "";
};

std::string Found = FindLib(LibrarySearchDirs);
if (Found.empty())
  Found = FindLib(StandardSearchDirs);
if (!Found.empty())
  return Found;
// report error
```


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:127
+    if (BaseName.endswith(".o"))
+      FullBaseName = BaseName.str();
+    else
----------------
smeenai wrote:
> sameerarora101 wrote:
> > smeenai wrote:
> > > I would construct a `Twine` and have `searchForFile` take `const Twine &` instead.
> > Seems like I can't construct a `Twine` before the `if` condition and modify it appropriately in the corresponding branches (which is needed so as to have the `Twine` variable in scope after the if condition). Furthermore, I also cannot directly call `searchForFile(<Twine>);` within each branch and assign the result to `Expected<std::string> FullPath`, as I cannot declare `Expected<std::string> FullPath;` before the `if` branch.
> > 
> > Sorry if I didn't get what you were trying to explain here ?
> > 
> >   
> Hmm, good points. Lemme think if there's anything else we can do here.
How about something like:
```
Expected<std::string> FullPath = searchForFile(BaseName.endswith(".o") ?
                                               BaseName.str() :
                                               "lib" + BaseName + ".a");
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85540