[PATCH] D47505: [llvm-strip] Add -o option

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/strip-all.test:6-7
 
-# We run yaml2obj again rather than copy %t to avoid interfering
-# with llvm-objcopy's test (which potentially could have corrupted/updated the binary).
+# Verify that the previous llvm-objcopy's run has not modified the input.
+# RUN: cmp %t %t3
 
----------------
alexshap wrote:
> jhenderson wrote:
> > I thought that we decided in a previous review not to do this, but instead to do what the old test is doing? Or am I misremembering?
> I think you are misremembering. We decided to avoid what the old test was doing. {F6291787} The motivation was (if I'm not mistaken) that it's not really necessary to run yaml2obj twice and then do text matching twice. 
Fair enough! I knew we'd decided to do it one way around, and not the other, so I was surprised to see a change.


================
Comment at: test/tools/llvm-objcopy/strip-all.test:12
 
+# RUN: cp %t %t4
+# RUN: llvm-strip %t4 -o %t5
----------------
I wonder if we need this extra copy here? We could probably just use %t at this point, as we have checked that it hasn't been modified. I'm not bothered though if you prefer it.


================
Comment at: tools/llvm-objcopy/StripOpts.td:11-12
+defm output : Eq<"o">,
+              MetaVarName<"output">,
+              HelpText<"Output file">;
+
----------------
paulsemel wrote:
> jhenderson wrote:
> > paulsemel wrote:
> > > what about :
> > > ```
> > >  MetaVarName<"file">,
> > >  HelpText<"Output stripped file into <file>">;
> > > ```
> > I think we should avoid mentioning "stripped" in the help text, because it's possible even now to use switches to prevent any actual stripping from occurring.
> Fair enough, but I think this might still be changed to be more verbose :)
Perhaps "Write output to <file>"?


Repository:
  rL LLVM

https://reviews.llvm.org/D47505