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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Overall it's a worthwhile direction - but please split this into smaller patches. General renamings separate, adding the functionality and using it in one place (preferably one that doesn't require function signature changes - one that already has the DWARFContext available, etc) & then a separate patch probably for each different error handling location that requires a function signature change. I think it'll be easier to review that way & make it clearer what changes were needed and how the error handling is being layered, etc.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:94-96
+  std::function<void(Error E)> RecoverableErrorHandler = [](Error E) {
+    defaultErrorHandler(std::move(E));
+  };
----------------
Does this need to be wrapped in a lambda here?


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:355
+  };
+  std::function<void(Error E)> getRecoverableErrorHandler() {
+    return RecoverableErrorHandler;
----------------
avl wrote:
> jhenderson wrote:
> > Having a setter and getter for these seems a bit verbose, especially when you've got cases where you use the getter and then immediately call the return. Is there any particular reason to hide the handlers behind the interface?
> There is no particular reason. Having getter/setter allows to add some code like checking for nullptr, or dumping, or something else... If it looks verbose I would remove it. 
@jhenderson : perhaps/did you mean "Is there any particular reason /not/ to hide the handlers behind the interface" - ie: I think you might've been suggesting that, rather than having "getRecoverableErrorHandler" the interface would have "emitRecoverableError(ERror E)" that calls the handler. Not exposing the handler itself to users of DWARFContext? Is that what you were/are suggesting? It sounds like a reasonable suggestion to me - at least worth asking/considering.

As for the setters - they seem to be uncalled right now? (just searching for them in this phab page doesn't show up any other instance of their names) - so probably best to leave them out until they're needed & we can discuss the merits of their use then?


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:307
                       const DWARFContext &Ctx, const DWARFUnit *U,
-                      function_ref<void(Error)> RecoverableErrorCallback);
+                      function_ref<void(Error)> RecoverableErrorHandler);
 
----------------
Please separate this sort of renaming into  a pre or post patch to make this review easier/more targeted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74308