[PATCH] D120731: [llvm] add -o flag to llvm-bitcode-strip

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.
Herald added a project: All.

I was going to suggest that the option should be documented in the llvm-bitcode-strip command guide, but it looks like that this documentation hasn't been written yet :(



================
Comment at: llvm/test/tools/llvm-objcopy/MachO/bitcode-strip.test:1
+## Test output flag is required
+
----------------



================
Comment at: llvm/test/tools/llvm-objcopy/MachO/bitcode-strip.test:4
+# RUN: yaml2obj %s -o %t
+# RUN: not llvm-bitcode-strip %t
+# RUN: llvm-bitcode-strip %t -o %t2
----------------
You shouldn't just run a naked `not ...` without checking the error message produced by `llvm-bitcode-strip`


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/bitcode-strip.test:5
+# RUN: not llvm-bitcode-strip %t
+# RUN: llvm-bitcode-strip %t -o %t2
+
----------------
You should check that `%t2` contains the output you expect, whatever that may be.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/bitcode-strip.test:29
+    flags:           0
+    Sections:
+      - sectname:        __text
----------------
I suspect you only need 0 or 1 sections for the test case to be sufficient?


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOptions.cpp:1211
+
+  if (!InputArgs.hasArg(BITCODE_STRIP_output)) {
+    return createStringError(
----------------
Are you sure you want this behaviour? Prior to this patch, llvm-bitcode-strip will modify things in-place, like llvm-strip does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120731