[PATCH] D74481: [Debuginfo][NFC] Create common error handlers for DWARFContext.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DIContext.h:208
+
+  std::function<void(Error)> RecoverableErrorHandler = nullptr;
+  std::function<void(Error)> WarningHandler = nullptr;
----------------
avl wrote:
> JDevlieghere wrote:
> > Could we make the default handlers static members of the DWARFContext and use those instead of nullptr?
> i.e. to make:
> 
> 
> ```
> class DWARFContext {
> ...
>  static std::function<void(Error)> RecoverableErrorHandler = defaultErrorHandler;
>  static std::function<void(Error)> WarningHandler = defaultWarningHandler;
> }
> 
> struct DIDumpOptions {
> .....
>   std::function<void(Error)> RecoverableErrorHandler = DWARFContext::RecoverableErrorHandler;
>   std::function<void(Error)> WarningHandler = DWARFContext::WarningHandler;
> };
> 
> ```
> 
> The goal of this refactoring is to allow to redefine error handlers. So that, when DWARFContext is used inside the linker, the linker could specify it`s own error handlers. There would be possible to provide custom error handlers to DWARFContext:  either by passing them in the constructor, either by giving them through setters (setRecoverableErrorHandler()) methods. So that concrete instance of  DWARFContext would have custom error handlers. That scheme would not work if RecoverableErrorHandler and WarningHandler were static methods of DWARFContext.
> 
> So, I propose that we would not do this.
> 
> 
> Additionally, DWARFContext is more specific class than DIDumpOptions. So it would not be good to refer it inside DIDumpOptions. It would probably be necessary to move ErrorHandlers into DIContext if that solution would be taken. 
Something I've been thinking about is whether something really high-level, e.g. Error.h, has some default handlers that can be used throughout LLVM. Configuring it on a system-wide level may not be right, but at least the defaults could then be used by all the different tools, unless there was a good reason not to. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74481