Review Request 112039: Add a CPU registers view.

[Vlas Puhov]

> On Aug. 21, 2013, 12:26 a.m., Milian Wolff wrote:
> > debuggers/gdb/registers/registersmanager.cpp, line 66
> > <http://git.reviewboard.kde.org/r/112039/diff/3/?file=183677#file183677line66>
> >
> >     foreach

I doesn't work. There is a lot of errors like: no type named ‘const_iterator’ in ‘const struct GDBMI::Value’


> On Aug. 21, 2013, 12:26 a.m., Milian Wolff wrote:
> > debuggers/gdb/registers/registercontroller_arm.cpp, line 96
> > <http://git.reviewboard.kde.org/r/112039/diff/3/?file=183673#file183673line96>
> >
> >     such errors should probably be shown as a kWarning()

What is the difference? Seems like nothing changes...
BTW this most likely never happen (there is no direct access to this function from user-land code). This check is just in case.


> On Aug. 21, 2013, 12:26 a.m., Milian Wolff wrote:
> > debuggers/gdb/registers/registersmanager.cpp, line 94
> > <http://git.reviewboard.kde.org/r/112039/diff/3/?file=183677#file183677line94>
> >
> >     this should probably never happen, or? Q_ASSERT then?

It can happen. For example when you debug an app on x86_64, then decide to try it out on your arm device (remote debugging).


> On Aug. 21, 2013, 12:26 a.m., Milian Wolff wrote:
> > debuggers/gdb/registers/registersmanager.cpp, line 153
> > <http://git.reviewboard.kde.org/r/112039/diff/3/?file=183677#file183677line153>
> >
> >     why delete later? If not required, use a QScopedPointer and .reset() etc.

Why? QT's "object trees" feature already used. 
I'm not very familiar with multithreading/task switching, but can't happen something like this:
user sent some action to registerController, and while it proceeding we delete the controller??


> On Aug. 21, 2013, 12:26 a.m., Milian Wolff wrote:
> > debuggers/gdb/registers/registersview.cpp, line 108
> > <http://git.reviewboard.kde.org/r/112039/diff/3/?file=183679#file183679line108>
> >
> >     should only be done if !groups.isEmpty.
> >     
> >     generally, can the other actions do anything sensible when no registerController is available? should maybe nothing be done in such a case?

Of course. setController is responsible for that. It completely disables register's widget if there is no RegisterController. So all these "if(m_registerController)" checks just in case. 


> On Aug. 21, 2013, 12:26 a.m., Milian Wolff wrote:
> > debuggers/gdb/registers/registersview.cpp, line 206
> > <http://git.reviewboard.kde.org/r/112039/diff/3/?file=183679#file183679line206>
> >
> >     this manual show/hide stuff looks wrong, can you add a screenshot of the gui and explain what you try to achieve here? Maybe you want a stacked widget?

I think one group at a time is not enough. Screenshots attached.


> On Aug. 21, 2013, 12:26 a.m., Milian Wolff wrote:
> > debuggers/gdb/registers/registersview.cpp, line 363
> > <http://git.reviewboard.kde.org/r/112039/diff/3/?file=183679#file183679line363>
> >
> >     you don't need the _ prefix here

How then? tableWidget(tableWidget), name(name) {} ??


- Vlas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112039/#review38237
-----------------------------------------------------------


On Aug. 20, 2013, 10:59 p.m., Vlas Puhov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112039/
> -----------------------------------------------------------
> 
> (Updated Aug. 20, 2013, 10:59 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> Supported architectures: x86/x86_64, arm v7(v6 should work too).
> Features: x86/x86_64: General registers/flags, segment, FPU registers read/write. XMM - read only.
> arm: General registers/flags, VFP single - read/write, other - read only.
> 
> Related changes:
> DisassembleWidget: removed startAddress and endAddress combo boxes so as to let to resize disassemble view, but the change address functionality is still there through context menu.
> FramestackModel: if first frame doesn't contain debug information don't set currentPosition to the first frame with debug information. It allows to keep synchronization between views(gdb, editor, disassemble widget and registers) otherwise all these views'll be in different states.
> 
> 
> This addresses bug 273152.
>     http://bugs.kde.org/show_bug.cgi?id=273152
> 
> 
> Diffs
> -----
> 
>   debuggers/gdb/CMakeLists.txt 32ef14f 
>   debuggers/gdb/debuggerplugin.cpp 3533cdb 
>   debuggers/gdb/disassemblewidget.h 81c25fc 
>   debuggers/gdb/disassemblewidget.cpp e1d6e8f 
>   debuggers/gdb/registers/registercontroller.h PRE-CREATION 
>   debuggers/gdb/registers/registercontroller.cpp PRE-CREATION 
>   debuggers/gdb/registers/registercontroller_arm.h PRE-CREATION 
>   debuggers/gdb/registers/registercontroller_arm.cpp PRE-CREATION 
>   debuggers/gdb/registers/registercontroller_x86.h PRE-CREATION 
>   debuggers/gdb/registers/registercontroller_x86.cpp PRE-CREATION 
>   debuggers/gdb/registers/registersmanager.h PRE-CREATION 
>   debuggers/gdb/registers/registersmanager.cpp PRE-CREATION 
>   debuggers/gdb/registers/registersview.h PRE-CREATION 
>   debuggers/gdb/registers/registersview.cpp PRE-CREATION 
>   debuggers/gdb/registers/registersview.ui PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112039/diff/
> 
> 
> Testing
> -------
> 
> Manual only for x86/x86_64 and armv7 architectures 
> 
> 
> File Attachments
> ----------------
> 
> Related changes in kdevplatform
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/08/12/framestackmodel.diff
> 
> 
> Thanks,
> 
> Vlas Puhov
> 
>

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