[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:1
+## Test strip using the same input file more than once.
+
----------------
strip -> llvm-strip

and add a brief explanation of what is expected.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/same-file-strip.test:17-20
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
----------------
Nit: remove the extra whitespace here and below so that each block has a lined-up column as far to the left as possible, i.e:

```
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_REL
  Machine: EM_X86_64
```


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:810
+          return createStringError(errc::invalid_argument, "cannot specify '-' as an input file more than once");
+        reportWarning("'" + Filename + "' was already specified");
+      }
----------------
abrachet wrote:
> rupprecht wrote:
> > jhenderson wrote:
> > > As you are planning on librarifying llvm-objcopy, be VERY wary about adding warnings directly in the code that might be eventually used in the library. If you haven't already, take a look at my lightning talk from last year's US Developers' meeting, which includes some tips and gotchas on this sort of thing:
> > > 
> > > https://www.youtube.com/watch?v=YSEY4pg1YB0
> > +1, if possible we should not be calling back into llvm-objcopy.cpp
> @jhenderson How would you recommend dealing with warnings in this case? I definitely agree with your sentiment and think failing at the local site is generally bad. Not sure how to deal with warnings though, llvm::Error is pretty binary it seems to me, either error or success.
The exact method for warnings probably depends on what you are planning on doing. In this particular case, I'd recommend probably passing in some form of handler (e.g. a callback function) which is defined by the caller. It would take a string (or possibly an llvm::Error) and report the warning in a manner applicable to the specific application. I'd then leave `reportWarning` in the original location and pass it in.

If you went the llvm::Error route, you could have the callback return an llvm::Error containing any unhandled issues, which is checked in this code to see if it should continue or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63122