[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/docs/CommandGuide/llvm-libtool-darwin.rst:65
+  :option:`-L` and before the default search path. The default search path
+  includes directories `/lib/`, `/usr/lib` and `/usr/local/lib`.
+
----------------
Nit: remove the trailing slash on `/lib/`


================
Comment at: llvm/test/tools/llvm-libtool-darwin/L-and-l.test:100
+
+# NOT-FOUND: error: cannot locate file for '[[FILE]]'
+
----------------
Can you test this for a name ending with `.a` as well?


================
Comment at: llvm/test/tools/llvm-libtool-darwin/L-and-l.test:102
+
+## Check that an error is thrown when the input library is not valid:
+# RUN: touch %t/dirname/not-valid.o
----------------
This one too?


================
Comment at: llvm/test/tools/llvm-libtool-darwin/L-and-l.test:111
+# RUN: llvm-ar cr %t/dirname/libfoo.o.a %t-input1.o
+# RUN: not llvm-libtool-darwin -static -o %t.lib -lfoo.o 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=NOT-FOUND -DFILE=foo.o
----------------
You need the `-L%t/dirname` as well, right?


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:106
+    }
+    return std::string();
+  };
----------------
I'd prefer an `Optional<std::string>` instead of having the empty string represent the absence.


================
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());
----------------
sameerarora101 wrote:
> 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?
`FileName` will have already had the `lib` and `.a` additions at this point though, right?


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:84
+
+static const std::vector<StringRef> StandardDirs{
+    "/lib/",
----------------
sameerarora101 wrote:
> smeenai wrote:
> > 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`.
> Seems like I cannot have `constexpr` with `std::array<std::string, 3>`.  It says, "constexpr variable cannot have non-literal type 'const std::array<std::string, 3>' (aka 'const array<basic_string<char>, 3>')".
Ah, oh well.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:127
+    if (BaseName.endswith(".o"))
+      FullBaseName = BaseName.str();
+    else
----------------
sameerarora101 wrote:
> 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.
Ah, I keep forgetting about ternary expressions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85540