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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
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");
+      }
----------------
jhenderson wrote:
> 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.
+1 to a callback that logs an Error, and use createFileError instead of manually crafting the filename into the error string

The callback mechanism described in the talk can be seen here: https://github.com/llvm/llvm-project/blob/master/llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp#L837


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:88
+  WithColor::warning(errs(), ToolName) << Message << "\n";
+  errs().flush();
+}
----------------
abrachet wrote:
> The objcopy::error functions explicitly flush errs, I don't know why though because it appears to be unbuffered, but I figured I would follow how the rest of the code does it.
yep, stderr is unbuffered and doesn't require flushing -- it could be removed from there too


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63122