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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap 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()]));
+  };
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76562