<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Sep 27, 2016, at 2:55 PM, Daniel Austin Noland <<a href="mailto:daniel.noland@gmail.com" class="">daniel.noland@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">Folks on the outside of the SB API’s need to be able to pass callbacks in. We don’t currently have any templates you need to instantiate or classes you need to override to go from outside the SB API to inside it. So whatever happens on the lldb_private side, for the SB API’s we’ll probably have to stick with void * & function pointers.<br class=""></blockquote><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">That is fair. I may be able to avoid the entire problem with llvm::function_ref<>. Will look into it.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></div></blockquote><div><br class=""></div><div>I do not think we want the SB API’s to require llvm header files.</div><div><br class=""></div><div>Jim</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" class="">5. Confusing or simply wrong documentation (e.g. Breakpoint::ResolveBreakpointInModules has arguments marked as @param[in] when the description of those arguments clearly indicates that they will be the output of the function).<br class="">6. We simply don't have a Finishpoint class (triggers callback at the conclusion of the function or scope)<br class=""><br class="">My plan:<br class=""><br class="">I would like to refactor this in a few phases.<br class=""><br class="">Phase 1: Low hanging fruit<br class="">--------------------------<br class=""><br class="">* Kick as much functionality as is reasonable from Breakpoint up to Stoppoint. At a minimum I would expand the Stoppoint API to include features which should logically be available to Breakpoint, Watchpoint, a hypothetical Finishpoint, and any future *point class. I would also try to refactor the Breakpoint support classes (e.g. BreakpointID) to derive from a new class (i.e. StoppointID) and kick functionality up the chain there as well. This would include methods like GetDescription, SetOptions, GetOptions, AddName, and so on.<br class=""></blockquote>Again, most of the overlap isn’t actually in the breakpoint and watchpoint classes, but in the Options classes. After all, the setting of watchpoints and breakpoints are very different, but that they have conditions and callbacks are very similar. Keeping the setting part separate and sharing the reaction to hitting the stop points seems to more closely model the objects better.<br class=""><br class=""><blockquote type="cite" class="">* Review and clean the source docs<br class=""><br class="">* Expand test coverage where that seems easy (perhaps there are tests hiding somewhere I didn't see?)<br class=""></blockquote>test/testcases/functionalities/breakpoint<br class=""><br class="">There are lots of tests there, but feel free to add tests as you go.<br class=""><br class=""><blockquote type="cite" class="">I would try to break private APIs minimally if at all. SB api will not be broken.<br class=""><br class="">This should go a long ways toward resolving problems 1, 2, 3, and 5.<br class=""><br class="">Phase 2: Restructure / Modernize the Private API / Implementation<br class="">-----------------------------------------------------------------<br class=""><br class="">* Change Error& out parameters to Expected<ReturnType>.<br class=""></blockquote>Again, this is a larger policy change. We shouldn’t make different sections of lldb handle errors differently.<br class=""><br class=""><blockquote type="cite" class="">* Get rid of all the const char* vars / args in favor of a better string type (which?)<br class=""></blockquote>StringRef’s seem to be the vogue these days.<br class=""><br class=""><blockquote type="cite" class="">* Prefer explicitly deleted copy ctor / assignments over multiline macro DISALLOW_COPY_AND_ASSIGN<br class="">* Try to reduce the (perhaps excessive) use of friendship between the support classes. For instance, is it necessary to give BreakpointLocation access to private data members from BreakpointLocationList, Process, BreakpointSite, and StopInfoBreakpoint? Wouldn't it be better to expand the functionality of those other classes?<br class=""><br class="">A more significant change could be a rewrite of the callback functionality.<br class=""><br class="">There are some problems with the way callbacks are implemented in terms of maintainability and extensibility. I think that using templates and simple inheritance we could shift to using callback objects instead of function pointers. I have a crude code sketch of this plan in the works now, and I will submit that if there is interest in this idea. Essentially, the idea is to let the user define their own Breakpoint or Watchpoint (or whatever) child class which overrides a pure virtual run method from a parent StoppointCallback templated class. The template parameter of StoppointCallback would allow us to define a std::unique_ptr<UserData> member where the user can hang any data they want for the callback. That saves us from void pointers (which I find very awkward).<br class=""><br class="">template <class UserData><br class="">class StoppointCallback {<br class="">private:<br class=""> std::unique_ptr<UserData> m_data;<br class="">// ...<br class="">};<br class=""><br class="">I also think that such a decision requires significant thought before we pull that trigger. It does constitute a significant addition to the SB api, tho I don't think it would break the SB api per se since we can always use overloading and template specialization to keep the old functionality. On the other hand, ABI compatibility may be a problem. I don't know that much about SWIG or the needs of LLDB from a ABI stability perspective, so please speak up if I'm suggesting something crazy.<br class=""></blockquote>We’ve been pretty strict about not requiring subclassing or specialization of SB API’s classes to use them. We’re serious about maintaining binary compatibility across lldb versions.<br class=""><br class=""><blockquote type="cite" class="">That said, it is somewhat difficult to keep the API consistent between Breakpoint and Watchpoint using function pointers; the signature associated with each will differ (e.g. Watchpoint should provide old and new values of the subject variable, while that does not apply to Breakpoint at all). I welcome any suggestions which allow us to avoid logic duplication. My goto just happens to be templates here.<br class=""></blockquote>I think you would call back into the watchpoint to get the old and new values. I don’t think you would need more than the frame passed into the watchpoint callback, and the location.<br class=""></blockquote><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class=""></span></div><br class="Apple-interchange-newline"></div></blockquote></div><br class=""></body></html>