[PATCH] D120669: [llvm] add -r functionality to llvm-bitcode-strip

[Richard Howell via Phabricator via llvm-commits]

rmaz marked 4 inline comments as done.
rmaz added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOptions.cpp:1225-1229
+  // We only support -r for now, which removes all bitcode sections
+  if (Error E = Config.ToRemove.addMatcher(NameOrPattern::create(
+          "__LLVM,__bundle", MatchStyle::Literal, ErrorCallback))) {
+    return std::move(E);
+  }
----------------
jhenderson wrote:
> rmaz wrote:
> > jhenderson wrote:
> > > Period at end of sentences. Also, no braces for single-line if statements.
> > > 
> > > I think you need additional sections in your test case with a section in the `__LLVM` segment with a different name, and a `__bundle` section in a non-LLVM segment, to show that it's only the combination of the two that is removed.
> > > 
> > > It may also be a good idea to show what happens (in a test case) when the error callback is triggered in this code path.
> > I don't believe you can trigger the error callback on this code path as we are using a `MatchStyle::Literal`.
> Thanks. Can the if statement ever fail though? (I'm not sure if the `ErrorCallback` and returned `Error` are tied together or not). If not, I'd change it to `cantFail`.
The if statement can only fail if the Matcher fails to create, which for a literal matcher cannot happen. Updated to `cantFail`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120669