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

[Alexey Lapshin via Phabricator via llvm-commits]

avl 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;
----------------
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.


================
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();
 }
----------------
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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87987