Review Request 112039: Add a CPU registers view.

[Vlas Puhov]

> On Aug. 20, 2013, 12:28 a.m., Kevin Funk wrote:
> > debuggers/gdb/registers/registercontroller.cpp, line 170
> > <http://git.reviewboard.kde.org/r/112039/diff/2/?file=183539#file183539line170>
> >
> >     The variable 'ok' is unused, this will result in warning. More occurrences in this patch, please fix those.

Hmm... It's kinda used: ...toUInt(&ok, 16); 
I've compiled the whole patch with gcc. There were no warnings at all.
If you use some other compilers and there is actually warnings, please let me know.


> On Aug. 20, 2013, 12:28 a.m., Kevin Funk wrote:
> > debuggers/gdb/registers/registercontroller.cpp, line 152
> > <http://git.reviewboard.kde.org/r/112039/diff/2/?file=183539#file183539line152>
> >
> >     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.

I don't get it, really. Why is it so bad?? 
It did it like that because it was very convenient to write something like this:
 return convertValuesForGroup (fillValuesForRegisters (getRegistersFromGroupInternally (group) ), format );
Now it takes whole 4 lines of code.

a) It doesn't fit because I don't allocate any memory. And something like this:
getRegistersFromGroupInternally (&group); fillValuesForRegisters(&group)...
IMO looks very odd(because it's not obvious why is it needed to pass in an address not a reference)
b)Nor does this. 
  Because RegistersGroup is not a simple data type. Creating new copies of this object takes some time/resources, so I don't want to create zillion copies of it without real necessity.
Or is it not an issue here with QT's implicit sharing feature?

So, I pass in a reference and explicitly denote that it's an output parameter.
PS: Still I'd like to know why is it not a good idea to pass around a reference. Because I can't find any reasonable explanation.:(


- Vlas


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


On Aug. 19, 2013, 9: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, 9: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/20130820/b15f2e02/attachment.html>