Review Request 112039: Add a CPU registers view.

Milian Wolff mail at milianw.de
Tue Aug 20 20:26:29 UTC 2013 

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



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

    this is not used in this function, is the check correct?



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

    either check the OK value below, or remove it and pass 0 instead to toUInt.



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

    no c-style casts please, use static_cast instead.



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

    make this a static const:
    
    static const QStringList registerGroups = QStringList() << enumToString(...) << ...;
    return registerGroups;



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

    same as above



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

    you can reuse the group string here instead of doing another enum-to-string conversion



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

    such errors should probably be shown as a kWarning()



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

    I always welcome a short comment with the expected format above a regexp. That greatly simplifies maintenance in the future if one needs to compare "what is this trying to do" with "what is the new format like" or similar.
    
    I.e. something simple like this:
    
    // foo(bar, asdf, ...)
    QRegExp rx("^[^(]+\([^,\)]+(,[^\)])*\)");



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

    here and everywhere else: RegisterGroups is an enum so don't use const&, just pass it by value like you'd do for an int or bool.
    
    also, considering how often this function is being called, I think it would be better to implement the lookup with a static map to save lookups. I.e.:
    
    {
      static QString strings[LAST_REGISTER] = {
        "General", "Flags", "VFP single-word", ...
      };
    }
    
    This of course requires you to add the LAST_REGISTER enumerator.
    
    Also: Is the return type here ever shown to the user? If so, doesn't this need to be translated then?



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

    maybe to reduce the confusion, rename these enums to ArmRegisterGroups and x86RegisterGroups or similar?



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

    static const see above



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

    remove this



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

    here and below: how often will these init functions be called? Can we share memory by init'ing them once in statics and using Qt's implicit sharing system?



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

    this comment should go below the arch = x86;



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

    kWarning() << "Unsupported architecture" << arch;
    
    but I just see that this is repeated further below where I think the warning is better placed. remove the kDebug's here



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

    foreach



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

    break on new line



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

    use mem-initializer list



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

    this should probably never happen, or? Q_ASSERT then?



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

    why delete later? If not required, use a QScopedPointer and .reset() etc.



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

    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?



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

    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?



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

    you don't need the _ prefix here



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

    here and above: break on new lines please
    
    Foo::Foo(...)
    : bar()
    { }


- Milian Wolff


On Aug. 20, 2013, 6: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, 6: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/20130820/bfc4f7b7/attachment-0001.html>

More information about the KDevelop-devel mailing list