[PATCH] D74308: [Debuginfo][NFC] Unify error reporting routines inside DebugInfoDWARF.

[Alexey Lapshin via Phabricator via llvm-commits]

avl marked 4 inline comments as done.
avl added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:115
+               std::string DWPName = "",
+               std::function<void(Error)> TheRecoverableErrorHandler =
+                   WithColor::defaultErrorHandler,
----------------
jhenderson wrote:
> This can just be `RecoverablErrorHandler`. No need for the `The` - constructor arguments can be named the same as the thing they're initialising. Same goes throughout most of the other functions too.
Ok.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:362
   /// message and returns Continue, so DWARF context ignores the error.
-  static ErrorPolicy defaultErrorHandler(Error E);
+  static ErrorPolicy objectFileErrorsHandler(Error E);
 
----------------
jhenderson wrote:
> objectFileErrorsHandler -> objectFileErrorHandler
ok.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:366-367
   create(const object::ObjectFile &Obj, const LoadedObjectInfo *L = nullptr,
-         function_ref<ErrorPolicy(Error)> HandleError = defaultErrorHandler,
-         std::string DWPName = "");
+         function_ref<ErrorPolicy(Error)> TheObjectFileErrorsHandler =
+             objectFileErrorsHandler,
+         std::string DWPName = "",
----------------
jhenderson wrote:
> I don't think you need to pass this in as well as a RecoverableErrorHandler. The two shouldn't need to diverge, I think. Rather, have ObjectFileErrorHandler be defined in terms of RecoverableErrorHandler, as suggested below.
There are two problems here: 

1. Using RecoverableErrorHandler for implementing ObjectFileErrorHandler.

   ObjectFileErrorHandler is a static function member, thus it could not be implemented using non-static function member RecoverableErrorHandler.

2. Passing one Error handler instead of two error Handlers. i.e.:

   instead of passing objectFileErrorsHandler and RecoverableErrorHandler into DWARFContext::create() pass only one handler.

   As far as I understand original logic for DWARFContext::create(), there should be possible to pass handler which would stop processing if error occurred. i.e. it would return ErrorPolicy::Halt. That behavior is important for the creation function. But for the other DWARFContext errors that policy is not supported. It could probably be incorrect to use handler which returns ErrorPolicy::Halt for other DWARFContext errors.

   Since error logic for create() method and for other places differs - it looks correct to use different handlers. What do you think?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1432
 
-ErrorPolicy DWARFContext::defaultErrorHandler(Error E) {
+ErrorPolicy DWARFContext::objectFileErrorsHandler(Error E) {
   WithColor::defaultErrorHandler(std::move(E));
----------------
jhenderson wrote:
> Shouldn't this function use the provided error handler rather than the default one?
> 
> In fact, maybe this should just be an inline lambda now.
since objectFileErrorsHandler is a static function - it could not use provided error handler(which is not-static member - RecoverableErrorHandler)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74308