Review Request 121484: debugger: IBreakpointController refactoring (incompatible API changes)

[Nicolai Hähnle]

> On Dez. 16, 2014, 11:52 vorm., Milian Wolff wrote:
> > debugger/breakpoint/breakpointmodel.cpp, line 226
> > <https://git.reviewboard.kde.org/r/121484/diff/2/?file=333193#file333193line226>
> >
> >     that's odd code ;-) just call debugController directly, no need to go from controller to session to controller, or?

They are two different controllers: there's an IDebugController, which owns the (persistent) breakpoint and variables model and knows about the current IDebugSession, which in turn holds the IBreakpointController. The IDebugController has KDevelop-lifetime and is implemented in kdevplatform, while the IBreakpointController only lives while a debug session is going on and is implemented in the debugger plugin.


> On Dez. 16, 2014, 11:52 vorm., Milian Wolff wrote:
> > debugger/interfaces/ibreakpointcontroller.cpp, line 208
> > <https://git.reviewboard.kde.org/r/121484/diff/2/?file=333197#file333197line208>
> >
> >     still, please rename this to get rid of the ugly `this->`:
> >     
> >         BM* model = breakpointModel();
> >     
> >     And, btw, I very much disagree with your aversion of `auto`, but if you insist on typing more I won't hold you up :)

Renamed.

It's not an aversion to auto, it's having been burnt too many times by the imperfections of KDevelop's code parsing - which is great, and the main reason for me to use KDevelop in the first place, but it's simply not yet good enough (I am sometimes an insufferable perfectionist). If the parser correctly handled everything that g++ compiles, I would lose my reservations against using auto everywhere :)

Case in point: in the issue before this one, where you suggest the "auto& dirty", KDevelop master does not correctly deduce the type of dirty, and so code completion for dirty.insert fails. I used auto anyway in this case, so you will be able to take a look at it yourself ;)


> On Dez. 16, 2014, 11:52 vorm., Milian Wolff wrote:
> > debugger/breakpoint/breakpointmodel.h, line 93
> > <https://git.reviewboard.kde.org/r/121484/diff/2/?file=333192#file333192line93>
> >
> >     this, so far, is never actually used - is it? I.e. I cannot find a use of it where multiple flags are passed in one go. Is that something you really see the need for _now_? Or is it just overkill currently, and could be introduced once it actually is required? Or will you be using it directly in the GDB code?

The plan is to use it in the GDB code, where QSet<Breakpoint::Column> is currently used.


> On Dez. 16, 2014, 11:52 vorm., Milian Wolff wrote:
> > debugger/interfaces/ibreakpointcontroller.cpp, line 119
> > <https://git.reviewboard.kde.org/r/121484/diff/2/?file=333197#file333197line119>
> >
> >     auto& dirty = m_dirty[breakpoint];
> >         dirty.insert(BreakPoint::...);
> >         if (...) {
> >             dirty.insert(...);
> >         } ...

Good catch :)


- Nicolai


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121484/#review72116
-----------------------------------------------------------


On Dez. 14, 2014, 6:59 nachm., Nicolai Hähnle wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121484/
> -----------------------------------------------------------
> 
> (Updated Dez. 14, 2014, 6:59 nachm.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> Overall, I want to move the code into a shape where BreakpointModel stores all information about breakpoints as presented to the user, and IBreakpointController is a genuine thin interface with no code (or only shim code) in kdevplatform.
> 
> The changes are necessarily binary incompatible, but AFAIU this is okay in current kdevplatform and kdevelop master. There are also minor source incompatible changes; I have already updated kdevelop master so that it is compatible with them.
> 
> The next step after this set of changes will be to change the gdb plugin in kdevelop so that it no longer uses what is now marked as the "old API". This will reduce the interactions between kdevelop and kdevplatform. Once those changes have landed and are stable, the old API in kdevplatform can then be purged.
> 
> Besides the usual reviewing feedback and feedback on the general direction of these changes, I am particularly interested in:
> 
> 1. Are there other users of the debuggers/breakpoint API, and source/binary compatibility policies that I should be aware of?
> 
> 2. I introduced the Column and ColumnFlags enum, where the latter is intended to replace various sets by more compact bit fields. Is there a nice type-safe Qt way of having such a pair of enums, where one is for bitfield corresponding to sets of the other?
> 
> 
> Diffs
> -----
> 
>   debugger/breakpoint/breakpoint.h 23c690f243ec3a46dfb66fd220c620125fd07327 
>   debugger/breakpoint/breakpoint.cpp 7e6de84208050192b6af3242c0cabd5f5515b567 
>   debugger/breakpoint/breakpointdetails.cpp 40d90bc863b10df24adf7189e12265ceb434a690 
>   debugger/breakpoint/breakpointmodel.h 025e78e6d270f870a6ad7d4c526640a3a404f59c 
>   debugger/breakpoint/breakpointmodel.cpp 0810db2f4f844beee13861ce4ba7d91464956fb0 
>   debugger/breakpoint/breakpointwidget.h 66567869e6d1fe8bdb89669c99a8fd1286568630 
>   debugger/breakpoint/breakpointwidget.cpp 48e3a7e6c11f4953098b24121c66a32e934a6acb 
>   debugger/interfaces/ibreakpointcontroller.h 5da4f3efafe7c6e97fbfafe9b639e05d2e0478ea 
>   debugger/interfaces/ibreakpointcontroller.cpp 03a7546166a61a8be9c9b6def2dcb3596c17e82a 
> 
> Diff: https://git.reviewboard.kde.org/r/121484/diff/
> 
> 
> Testing
> -------
> 
> kdevelop gdb unit tests work
> 
> 
> Thanks,
> 
> Nicolai Hähnle
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20141216/a4a50416/attachment-0001.html>