[PATCH] D71302: [llvm-ar] Improve tool selection logic

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 2 inline comments as done.
MaskRay added a comment.

In D71302#1778295 <https://reviews.llvm.org/D71302#1778295>, @rupprecht wrote:

> > Update the heuristic to make all the following work as intended:
> > 
> > llvm-ar-9
> >  llvm-ranlib.exe
> > arm-pokymllib32-linux-gnueabi-llvm-ar
> > arm-pokymllib32-linux-gnueabi-llvm-ar-9
>
> This was manually verified, but can you add a test for it?
>  You could probably write a lit test like:
>
>   # RUN: mkdir -p bin
>   # RUN: cp llvm-ar bin/llvm-ar-9
>   # RUN: bin/llvm-ar-9 --help | FileCheck %s --check-prefix=AR
>   # RUN: cp llvm-ar bin/llvm-ranlib.exe
>   # RUN: bin/llvm-ranlib.exe --help | FileCheck %s --check-prefix=RANLIB
>   ...
>


The path of llvm-ar is not known. Even if it does, I don't know copying an executable to a directory that may be mounted with the noexec flag will work...



================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1184
+  auto Is = [](StringRef Tool) {
+    auto I = Stem.rfind(Tool);
+    return I != StringRef::npos &&
----------------
rupprecht wrote:
> Needs to be lower-cased (+ a test case -- e.g. `LLVM-AR.EXE` should work)
I think LLVM-AR.EXE is a DOSism that we don't need to care in 2019.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1185-1186
+    auto I = Stem.rfind(Tool);
+    return I != StringRef::npos &&
+           (I + Tool.size() == Stem.size() || !isAlnum(Stem[I + Tool.size()]));
+  };
----------------
rupprecht wrote:
> This is pretty opaque, I think a more obvious algorithm should be used even if it's possibly more inefficient -- there's no need for this to be fast, it called at most 4 times.
> 
> Is the intention to match all these?
> "...ar$"
> "...ar-..."
> "...ar.exe"
Yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71302