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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:115
+               std::string DWPName = "",
+               std::function<void(Error)> TheRecoverableErrorHandler =
+                   WithColor::defaultErrorHandler,
----------------
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.


================
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);
 
----------------
objectFileErrorsHandler -> objectFileErrorHandler


================
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 = "",
----------------
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.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFContext.cpp:1432
 
-ErrorPolicy DWARFContext::defaultErrorHandler(Error E) {
+ErrorPolicy DWARFContext::objectFileErrorsHandler(Error E) {
   WithColor::defaultErrorHandler(std::move(E));
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74308