[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:7
+# RUN: rm -rf %t/dirname && mkdir -p %t/dirname
+# RUN: llvm-ar cr %t/dirname/archive.o %t-input1.o %t-input2.o
+
----------------
sameerarora101 wrote:
> smeenai wrote:
> > I'd prefer for a file ending in `.o` to actually be an object file and not an archive :)
> I just wanted to check that a library ending in `.o` is not prepended/appended with some additional string lol. How about the name `archive-object.o`?
I think the intent is, even though the option is `-l` (with the l presumably standing for library), you can still use it to specify input object files (so that you don't have to specify their full path). What I meant was to just have an object file and use `-l` to add that, instead of creating an archive and giving it a `.o` extension.


================
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
----------------
sameerarora101 wrote:
> smeenai wrote:
> > 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.)
> Thanks, added now.
> 
> For the second test case, I assumed you meant passing in a library with `-l` along with a normal input file? Or did you mean prepending `-l` to an input file name?
> 
> For the third case, libtool throws a warning (similar to what it does when we specify the same input file twice). I have added a comment about that in my test case above now.
Yup, that's what I meant for the second case.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85540