[PATCH] D76562: [llvm-objcopy] Recognize llvm-strip-$major as a tool name

[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:
> > 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.
@MaskRay thanks for your patience and for the quick fix.

0/ Would be good to add a comment explaining what strings this code is intended to match
(so that people don't need to look through the tests).

1/ I suggested Regex primarily because this way it seemed to me that the code would be self-documented / easier to reason about, thus we would avoid having such bugs in the future, although, perhaps, this is subjective.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76562