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

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked 11 inline comments as done.
sameerarora101 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
+
----------------
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`?


================
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
----------------
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.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:99
+    sys::path::append(Path, Dir, FileName);
+    StringRef PathString = Path;
+
----------------
smeenai wrote:
> Any reason for creating this variable? SmallString can be implicitly converted to StringRef instead, and it can also be converted to `std::string` explicitly (e.g. via `std::string(Path)`).
Ah forgot about the explicit conversion, thanks!


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:123
+  for (std::string s : Libraries) {
+    StringRef BaseName = StringRef(s);
+
----------------
smeenai wrote:
> One of the following should work, I think.
> 
> ```
> StringRef BaseName = s;
> StringRef BaseName(s);
> ```
Both work 😄


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

  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85540