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

[Alexey Lapshin via Phabricator via llvm-commits]

avl marked an inline comment as done.
avl added inline comments.


================
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 = "",
----------------
avl wrote:
> 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?
@jhenderson  James, what about passing two error handlers to "create" function?:


```
create(const object::ObjectFile &Obj, const LoadedObjectInfo *L = nullptr,
         function_ref<ErrorPolicy(Error)> ObjectFileErrorsHandler =
             objectFileErrorsHandler,
         std::string DWPName = "",
         std::function<void(Error)> RecoverableErrorHandler =
             WithColor::defaultErrorHandler,
         std::function<void(Error)> WarningHandler =
             WithColor::defaultWarningHandler);

```
Would it be OK ? 

As an alternative objectFileErrorsHandler default implementation could be removed at all, with the price that every usage place would need to create it by itself:


```
create(const object::ObjectFile &Obj, const LoadedObjectInfo *L = nullptr,
         function_ref<ErrorPolicy(Error)> ObjectFileErrorsHandler =
             [](Error E)->ErrorPolicy {
               WithColor::defaultErrorHandler(std::move(E));
               return ErrorPolicy::Continue;
             },
         std::string DWPName = "",
         std::function<void(Error)> RecoverableErrorHandler =
             WithColor::defaultErrorHandler,
         std::function<void(Error)> WarningHandler =
             WithColor::defaultWarningHandler);

```




```
    Context = DWARFContext::create(*Objects.second, nullptr,
                                   [](Error E)->ErrorPolicy {
                                     WithColor::defaultErrorHandler(std::move(E));
                                     return ErrorPolicy::Continue;
                                   }, Opts.DWPName);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74308