[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:23
+# RUN: rm -rf %t/dirname && mkdir -p %t/dirname
+# RUN: llvm-ar cr %t/dirname/libsingle-archive.a %t-input2.o
+
----------------
Maybe worth naming archives with a single member in them after the member's name, e.g. here `libinput2.a`

I think that helps readability, also possibly reusability (see below).


================
Comment at: llvm/test/tools/llvm-libtool-darwin/L-and-l.test:57-58
+## in them, the file from the first directory is selected.
+# RUN: llvm-ar cr %t/dirname/libmultiple-dirs.a %t-input1.o
+# RUN: llvm-ar cr %t/otherDirname/libmultiple-dirs.a %t-input2.o
+
----------------
Rather than create two new archives here, you could just create one, in `otherDirname`, containing input1.o. You already have a library in `dirname` containing input2.o, so if you name the new one the same, you can use that one too.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/L-and-l.test:109
+
+## Check that 'lib' and '.a' are not added to a file ending in '.o':
+# RUN: llvm-ar cr %t/dirname/libfoo.o.a %t-input1.o
----------------
Let's also add a test case with a file extension other than these two, e.g. `.ext` and show that the behaviour is specific to '.o'.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:121
+static Error processCommandLineLibraries() {
+  for (const StringRef BaseName : Libraries) {
+    Expected<std::string> FullPath = searchForFile(
----------------
It's not particularly idiomatic to put `const` with `StirngRef`.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:127
+    if (BaseName.endswith(".o"))
+      FullBaseName = BaseName.str();
+    else
----------------
smeenai wrote:
> 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.
I avoid them in most cases, but this seems like a sensible usage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85540