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

Tue Feb 11 01:00:02 PST 2020

jhenderson added inline comments.

================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:355
+  };
+  std::function<void(Error E)> getRecoverableErrorHandler() {
+    return RecoverableErrorHandler;
----------------
dblaikie wrote:
> 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?
I don't think it's what I meant when I wrote that comment, but I think your suggestion manifesting itself as a half-formed thought was what triggered me to write a comment at all! I agree that an "emitRecovarableError(Error E)" signature would make a lot of sense - all it would do would be to call the handler with the passed-in error.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74308