[PATCH] D76562: [llvm-objcopy] Support llvm-strip-11 as argv[0]

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:335
+    return I != StringRef::npos &&
+           (I + Tool.size() == Stem.size() || !isAlnum(Stem[I + Tool.size()]));
+  };
----------------
alexshap wrote:
> MaskRay wrote:
> > alexshap wrote:
> > > What would you think about smth like this:
> > > 
> > >   const llvm::Regex StripRegex("^(llvm-)?strip(.*)$"); 
> > >   const llvm::Regex InstallNameToolRegex("^(llvm-)?install-name-tool(.*)$");
> > >   ...
> > >   if (StripRegex.match(Tool))
> > >     ...
> > >   else if (InstallNameToolRegex.match(Tool))
> > >     ...
> > > 
> > The current code is to make it similar to D71302 llvm-ar
> > 
> > We need to support something like `x86_64-unknown-linux-gnu-strip`
> 0/ the title of the diff "Support llvm-strip-11 as argv[0]" seems to be a little bit confusing.
> 1/ In any case - here and in D71302 the matching logic seems to be a tiny bit more complicated than it can be 
> (from the readability perspective). 
> Depending on what you want - if we need a strict well-controlled robust way of matching specific name patterns I'd probably use a regular expression rather than some ad-hoc code, if a simple match is good enough - i suspect StringRef::contains / StringRef::contains_lower could do their job here. Though I don't insist.
D71302 has to be complex because `"lib"` can be in the target triple ("arm-pokymllib32-linux-gnueabi-llvm-ar"). We need to guard against such cases. The logic is slightly more rubust. It can work with `powerpc64-stripinvalid-freebsd13-objcopy` (not very unrealistic).

I actually think Regex.h can increase code complexity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76562