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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht requested changes to this revision.
rupprecht added a comment.
This revision now requires changes to proceed.

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



================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:1184
+  auto Is = [](StringRef Tool) {
+    auto I = Stem.rfind(Tool);
+    return I != StringRef::npos &&
----------------
Needs to be lower-cased (+ a test case -- e.g. `LLVM-AR.EXE` should work)


================
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()]));
+  };
----------------
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"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71302