Review Request 112039: Add a CPU registers view.

Kevin Funk krf at gmx.de
Mon Aug 19 20:28:08 UTC 2013 

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



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

    const-ref the parameters
    
    (Note: There are many other hunks in this patch where you could use const-refs, I'm not highlighting those. Please fix them)



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

    Something is wrong with this method.
    
    You pass a reference + return the reference in the normal case. So there are basically two ways to get the result out of your method. You have to decide:
    a) Pass in a ptr, modify that one, return void.
    b) Pass in a const-ref, use that one and return a copy of the result.
    
    This is the style used throughout Qt. Passing around references is (as Milian noted) generally a bad idea.
    
    There are more occurrences of that return-reference-pattern in your diff, please fix those accordingly.



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

    The variable 'ok' is unused, this will result in warning. More occurrences in this patch, please fix those.



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

    Early-return?



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

    const-ref can be used inside foreach-loops.
    Multiple occurrences, please fix.



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

    Blank line, please remove. Multiple occurrences.



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

    Early-return?



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

    Why's that an int? Turn into RegistersFormat (enum) and you can get rid off the C-style casts in your code.



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

    No need to cast, no?



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

    This does not look future-proof nor easy to understand.
    
    Connect to the individual signals of the actions and handle them separately instead of handling them all in one slot.
    
    You can connect(...) inside RegistersView::contextMenuEvent in the foreach loop and inside RegistersView::addItemToFormatSubmenu.
    
    (E.g. one slot for the format submenu actions, one slot for switching the groups and "Update")



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

    Use an early-return here for better readability
    
    if (group.isEmpty()) {
      return t;
    }
    (...)
    
    (Same applies to other methods in this TU)
    



debuggers/gdb/registers/registersview.ui
<http://git.reviewboard.kde.org/r/112039/#comment28214>

    -> RegistersView


Some general notes:

Please get rid off the additional whitespace around brackets in your code (refer to the Qt coding style guide).

E.g. your class RegisterControllerGeneral_x86 would turn into:

class RegisterControllerGeneral_x86 : public IRegisterController
{
public:
     virtual QStringList namesOfRegisterGroups() const;
public slots:
     virtual void updateRegisters(const QString& group = QString());
protected:
     RegisterControllerGeneral_x86(QObject* parent, DebugSession* debugSession = 0);
(...)
};

- Kevin Funk


On Aug. 19, 2013, 5:45 p.m., Vlas Puhov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112039/
> -----------------------------------------------------------
> 
> (Updated Aug. 19, 2013, 5:45 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/registers/registersview.h PRE-CREATION 
>   debuggers/gdb/registers/registersmanager.cpp PRE-CREATION 
>   debuggers/gdb/registers/registersmanager.h PRE-CREATION 
>   debuggers/gdb/registers/registercontroller_x86.cpp PRE-CREATION 
>   debuggers/gdb/registers/registercontroller_x86.h PRE-CREATION 
>   debuggers/gdb/registers/registercontroller_arm.cpp PRE-CREATION 
>   debuggers/gdb/registers/registercontroller_arm.h PRE-CREATION 
>   debuggers/gdb/registers/registercontroller.cpp PRE-CREATION 
>   debuggers/gdb/registers/registercontroller.h PRE-CREATION 
>   debuggers/gdb/disassemblewidget.cpp e1d6e8f 
>   debuggers/gdb/disassemblewidget.h 81c25fc 
>   debuggers/gdb/debuggerplugin.cpp 3533cdb 
>   debuggers/gdb/CMakeLists.txt 32ef14f 
>   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/20130819/ac6942a5/attachment-0001.html>

More information about the KDevelop-devel mailing list