Review Request 112039: Add a CPU registers view.

Fri Aug 16 07:22:13 UTC 2013

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

half-finished review, but there is probably more you can deduce from the comments I made here. I'll take another look next week then, probably on thursday.

cheers and thanks for working on this!

debuggers/gdb/disassemblewidget.h
<http://git.reviewboard.kde.org/r/112039/#comment28051>

    const& the qstring, don't inline the function

debuggers/gdb/disassemblewidget.cpp
<http://git.reviewboard.kde.org/r/112039/#comment28052>

    already in the header included

debuggers/gdb/disassemblewidget.cpp
<http://git.reviewboard.kde.org/r/112039/#comment28053>

    just "registers/registersmanager.h"

debuggers/gdb/disassemblewidget.cpp
<http://git.reviewboard.kde.org/r/112039/#comment28054>

    yes, this is very ugly. pass around the receiver.

debuggers/gdb/disassemblewidget.cpp
<http://git.reviewboard.kde.org/r/112039/#comment28055>

    use member initializer list for that, give m_s a proper name (m_splitter)

debuggers/gdb/registers/registercontroller.h
<http://git.reviewboard.kde.org/r/112039/#comment28058>

    repeated below, just move the forward declaration down there.

debuggers/gdb/registers/registercontroller.h
<http://git.reviewboard.kde.org/r/112039/#comment28057>

    mark all these structs as movable using Q_DECLARE_TYPEINFO(GDBDebugger::..., Q_MOVABLE_TYPE), put that at the end of the file (outside the namespace)

debuggers/gdb/registers/registercontroller.h
<http://git.reviewboard.kde.org/r/112039/#comment28056>

    remove these two constructors, use

    Register foo = { foo, value };

    To achieve what you want.

debuggers/gdb/registers/registercontroller.h
<http://git.reviewboard.kde.org/r/112039/#comment28069>

    dont return const&

debuggers/gdb/registers/registercontroller.h
<http://git.reviewboard.kde.org/r/112039/#comment28070>

    dont return const&

debuggers/gdb/registers/registercontroller.h
<http://git.reviewboard.kde.org/r/112039/#comment28059>

    const&

debuggers/gdb/registers/registercontroller.h
<http://git.reviewboard.kde.org/r/112039/#comment28060>

    don't inline this.

debuggers/gdb/registers/registercontroller.h
<http://git.reviewboard.kde.org/r/112039/#comment28061>

    here neither

debuggers/gdb/registers/registercontroller.h
<http://git.reviewboard.kde.org/r/112039/#comment28063>

    here and below, remove the "get" prefix.

    also, do not return a reference

debuggers/gdb/registers/registercontroller.h
<http://git.reviewboard.kde.org/r/112039/#comment28067>

    this api is strange, why is the registersgroup not filled on construction? why does one need to call this function?

    if it stays in, don't return a reference and take a reference, return void and take a ptr.

    if it's just a helper function you want to use to clear up the code structure, make it a file-local helper function, not a protected class-local one

debuggers/gdb/registers/registercontroller.h
<http://git.reviewboard.kde.org/r/112039/#comment28068>

    don't return a const&

debuggers/gdb/registers/registercontroller.h
<http://git.reviewboard.kde.org/r/112039/#comment28064>

    don't return a const&

debuggers/gdb/registers/registercontroller.h
<http://git.reviewboard.kde.org/r/112039/#comment28062>

    what does "really" mean here?

debuggers/gdb/registers/registercontroller.h
<http://git.reviewboard.kde.org/r/112039/#comment28065>

    don't return a const QString

debuggers/gdb/registers/registercontroller.h
<http://git.reviewboard.kde.org/r/112039/#comment28066>

    here and below: this is not required to be sorted, or? then use QHash instead of QMap

    for m_rawRegisterNames you should evne use a plain QStringList or QVector<QString> - the key range is never sparse and starts from zero, no?

debuggers/gdb/registers/registercontroller.cpp
<http://git.reviewboard.kde.org/r/112039/#comment28071>

    make this a QVector<QString> and just append new entries.

debuggers/gdb/registers/registercontroller.cpp
<http://git.reviewboard.kde.org/r/112039/#comment28072>

    if its a QVector then use indexOf and != -1

debuggers/gdb/registers/registercontroller.cpp
<http://git.reviewboard.kde.org/r/112039/#comment28073>

    quite a lot of debug output, is that still required and useful?

debuggers/gdb/registers/registercontroller.cpp
<http://git.reviewboard.kde.org/r/112039/#comment28074>

    this is ugly, remove it when you stop returning a const&

debuggers/gdb/registers/registercontroller.cpp
<http://git.reviewboard.kde.org/r/112039/#comment28075>

    remove this and put it into the foreach loop

debuggers/gdb/registers/registercontroller.cpp
<http://git.reviewboard.kde.org/r/112039/#comment28076>

    use foreach

debuggers/gdb/registers/registercontroller.cpp
<http://git.reviewboard.kde.org/r/112039/#comment28079>

    imo just call the argument "registers".

debuggers/gdb/registers/registercontroller.cpp
<http://git.reviewboard.kde.org/r/112039/#comment28078>

    foreach

debuggers/gdb/registers/registercontroller.cpp
<http://git.reviewboard.kde.org/r/112039/#comment28077>

    foreach

- Milian Wolff

On Aug. 12, 2013, 6:52 p.m., Vlas Puhov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112039/
> -----------------------------------------------------------
> 
> (Updated Aug. 12, 2013, 6:52 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/20130816/f2b94d82/attachment-0001.html>