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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFContext.h:355
+  };
+  std::function<void(Error E)> getRecoverableErrorHandler() {
+    return RecoverableErrorHandler;
----------------
avl wrote:
> avl wrote:
> > avl wrote:
> > > jhenderson wrote:
> > > > 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.
> > > Ah, OK, Thanks. I misinterpreted second usage ...
> > But... it looks like it does not work that way : 
> > 
> > doSomething(/*Callback=*/Context.emitRecoverableError);
> > 
> > error: reference to non-static member function must be called
> > 
> > It needs to be 
> > 
> > doSomething(/*Callback=*/[&](Error Err){Context.emitRecoverableError(std::move(Err))});
> @dblaikie David, How emitRecoverableError() should be used for this scenario ? :
> 
> 
> ```
> doSomething(/*Callback=*/Context.emitRecoverableError);
> ...
> void doSomething(function_ref<void(Error E)> Handler) {
> ...
>   Handler(createStringError("some error"));
> }
> 
> ```
> 
> Exactly this variant seems not working, since std::function/function_ref could not be assigned to member function. it is necessary to use proxy lambda function or use std::bind to make it working.
> 
> But that probably is not very convenient solution.
Yeah, fair enough - seems like there are enough places that want to pass the handler along to that don't have a DWARFContext directly accessible (& makes sense to just pass down the handlers, rather than the whole DWARFContext). Seems fine as-is (as an accessor for the handler), then. Maybe @jhenderson has other thoughts since it was his feedback originally.

Still seems like the setters should be removed if (as they appear to be) they are unused.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74308