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

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 5 inline comments as done.
abrachet added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test:24
+Sections:
+  - Name:   .text
+    Type:   SHT_PROGBITS
----------------
jhenderson wrote:
> 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").
Wasn't quite sure which white space you were referring to so I copied this from one of your tests.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.h:194
+parseStripOptions(ArrayRef<const char *> ArgsArr,
+                  std::function<void(Error)> RecoverableErrorCallback);
 
----------------
jhenderson wrote:
> 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.
How does this look? It is used down on line 816 of CopyConfig.cpp


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:57
   WithColor::error(errs(), ToolName) << Message << ".\n";
-  errs().flush();
   exit(1);
----------------
jhenderson wrote:
> 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).
You're right. I put it back.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63122