[PATCH] D63122: [llvm-strip] Error when using stdin twice

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test:24
+Sections:
+  - Name:   .text
+    Type:   SHT_PROGBITS
----------------
Not that it matters, but I'd usually line up each block independently (i.e. I think you could remove an extra space from everything inside "Sections").


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.h:194
+parseStripOptions(ArrayRef<const char *> ArgsArr,
+                  std::function<void(Error)> RecoverableErrorCallback);
 
----------------
I'd probably change the name to just "ErrorCallback", and put a note in the comment explaining what this function is supposed to do. Since it's recoverable, and the program might be able to continue, I think it makes sense to return `Error`, and let the parseStripOptions code check the return value to decide if it is safe to continue or not.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:57
   WithColor::error(errs(), ToolName) << Message << ".\n";
-  errs().flush();
   exit(1);
----------------
abrachet wrote:
> I removed this in this patch because Jordan said it could be removed. Is it okay to sneak this one in this patch or should I separate this?
Whilst I'd probably do it, strictly it should probably be in its own micro-patch that doesn't need reviewing (as long as you build and run the tests).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63122