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

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked 8 inline comments as done.
sameerarora101 added inline comments.


================
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'"),
----------------
jhenderson wrote:
> 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.
Yup, the behavior here is definitely what's documented. My first test case is testing this only where I pass in `-larchive-object.o`. Also, I have now changed that test by passing in `-l%basename_t.tmp-input1.o` instead of `archive-object` as per @smeenai's recommendation above.


================
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());
----------------
jhenderson wrote:
> 
it is not trying to locate `'%s'` but might add `lib` and `.a` to the string and try locating `lib%s.a`, right? That's what I meant up there. I can change it to what you're suggesting if it doesn't make sense?


================
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;
----------------
jhenderson wrote:
> 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
> ```
I like the lambda too, updated now. Thanks!


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:127
+    if (BaseName.endswith(".o"))
+      FullBaseName = BaseName.str();
+    else
----------------
jhenderson wrote:
> 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");
> ```
works, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85540