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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/bitcode-strip.test:8
 
+## Test output flag and action flag are required
+
----------------
Nit: comments should end with a period. Also, you can delete "output flag and" from this comment: the previous test case covers this point, so no need for this test case to explicitly mention that too.

I'd also suggest deleting the blank line between comment and run command, since the two are tied together, rather than the comment being a general comment about the whole test file.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/bitcode-strip.test:7-10
+## Test bitcode section removal
+
+# RUN: llvm-bitcode-strip -r %t -o %t2
+# RUN: llvm-readobj --sections %t2 | FileCheck %s
----------------
rmaz wrote:
> jhenderson wrote:
> > Usually, we'd have one test file per option, i.e. in one test, you'd test -o, and another test, you'd test what the -r operation does.
> The reason for using a single test is that you cannot use `-o` without `-r`, so all the `-o` test could do would be to test you get an error without `-r`.
Thanks. When I made this and related comments, I didn't realise that you wanted to make `-r` a cumpolsory argument. Given that other commands are planned, I'd suggest the following test file breakdown between this and D120731:
`bitcode-strip-basic.test` - contains tests that show the -o option is required and that at least one command is required. You don't need to show what the behaviour of that command is in this test, other than it suppresses the error.
`bitcode-strip-r.test` (or similar longer name spelling out what -r does) - contains tests that show the behaviour of `-r`.


================
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);
+  }
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120669