[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:355
+  };
+  std::function<void(Error E)> getRecoverableErrorHandler() {
+    return RecoverableErrorHandler;
----------------
avl wrote:
> jhenderson wrote:
> > 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.
> I did not clearly get idea of "emitRecoverableError(ERror E)". My understanting is that it allows to change this usage :
> 
> 
> ```
> public:  std::function<void(Error E)> RecoverableErrorHandler; // field of DWARFContext
> ...
> DWARFContext.RecoverableErrorHandler(E);  // emit error.
> 
> ```
> with this usage:
> 
> 
> ```
> DWARFContext.emitRecoverableError(E);  // emit error, RecoverableErrorHandler is not seen
> 
> ```
> But what about this usage : 
> 
> 
> ```
> func (std::function<void(Error E)> ErrorHandler)  // some function
>       { ErrorHandler(Error); }
> 
> 
> func(DWARFContext.RecoverableErrorHandler);  // pass handler to function
> ```
> 
> How would this case be resolved with emitRecoverableError() ? 
> 
> Something like this: 
> 
> ```
> func([&](Error E ) { DWARFContext.emitRecoverableError(std::move(E)); }   // pass handler to function
> ```
> 
> ?
I'm not sure I follow your comments I'm afraid. The suggestion is to replace `getRecoverableErrorHandler()` with the following function:
```
void emitRecoverableError(Error Err) {
  RecoverableErrorHandler(std::move(Err));
}
```
Usage would be either:
```
Context.emitRecoverableError(createStringError("some text"));
```
or
```
doSomething(/*Callback=*/Context.emitRecoverableError);
...
void doSomething(function_ref<void(Error E)> Handler) {
...
  Handler(createStringError("some error"));
}
```
depending on whether the handler needs to be passed into the function, or the Context can be accessed directly. Effectively, in the second case, the real error handler is wrapped in another function that calls it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74308