[PATCH] D87987: [llvm-objcopy][NFC] refactor error handling. part 3.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:114-117
+  virtual Error visit(const Section &Sec) override;
+  virtual Error visit(const OwnedDataSection &Sec) override;
+  virtual Error visit(const StringTableSection &Sec) override;
+  virtual Error visit(const DynamicRelocationSection &Sec) override;
----------------
avl wrote:
> jhenderson wrote:
> > No need to specify `virtual` when a function is marked as `override`.
> I mark them as virtual so that they look the same as following definitions - will remove them.
Thanks - the key is that they AREN'T the same so we should be highlighting this by making them look different!


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:59-63
 ErrorSuccess reportWarning(Error E) {
   assert(E);
   WithColor::warning(errs(), ToolName) << toString(std::move(E)) << '\n';
   return Error::success();
 }
----------------
avl wrote:
> jhenderson wrote:
> > This might be an indication that you should use an error handler after all. You don't really want your library code printing warnings directly itself.
> This reportWarning() function is exactly implementation(used by only llvm-objcopy.cpp) of the error handler. It is not seen by other parts of llvm-objcopy. It is passed as "llvm::function_ref<Error(Error)> ErrorCallback" to other parts. Thus, It could/should not be used by library.
> 
> 
Okay, thanks. That's good to know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87987