Review Request 112039: Add a CPU registers view.

[Milian Wolff]

On Saturday 24 August 2013 07:41:58 Vlas Puhov wrote:
> > 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#file183677l
> > > ine66>> > 
> > >     foreach
> 
> I doesn't work. There is a lot of errors like: no type named
> ‘const_iterator’ in ‘const struct GDBMI::Value’

Ugh, my bad. Leave it as is then.

> > 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#file183673l
> > > ine96>> > 
> > >     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.

kWarning is always shown, whereas kDebug can be turned off (see e.g. the 
kdebugdialog application). And if this is a "most likely never happen" case, 
maybe even make it a Q_ASSERT? If you do not expect this to happen, ensure 
that it never happens.

> > 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#file183677l
> > > ine94>> > 
> > >     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).

Ah, sure. leave it then.

> > 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#file183677l
> > > ine153>> > 
> > >     why delete later? If not required, use a QScopedPointer and .reset()
> > >     etc.
> 
> Why? QT's "object trees" feature already used.

But object trees are only freed when the parent is freed. Thus if you'd 
continuously "forget" pointers you essentially leak until the app is closed. 
So deleting explicitly when something is not needed anymore is a _very_ good 
practice.

> 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??

This code is not thread safe and not being used from multiple threads. And 
deleteLater is also not something which would help with threadsafety.

There are some (rare, bad) places where deleteLater is required, but this 
doesn't look like one. 

Generally, deleteLater goes through the event loop thus it does not come for 
free. If you don't need it, don't use it.

And I prefer QScopedPointer's since then you don't need to call delete 
explicitly and the memory management is more failsafe. This is in accordance 
to the "new" patterns which emerge from usage of Qt, Boost and C++11 smart 
pointers.

> > 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#file183679l
> > > ine108>> > 
> > >     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.

Such "just in case" code is imo a code smell. I rather prefer pedantic code 
which is littered with Q_ASSERT or similar to ensure that the code works as 
expected. This simplifies the code and later on when someone reads the code he 
can be sure of whats going on. If there are tons of if(...) checks one cannot 
be sure of when this pointer is going to be valid or not. If a function otoh 
checks that once and returns early we can know "ok, here it may be invalid and 
we don't do anything". 

> > 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#file183679l
> > > ine206>> > 
> > >     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.

OK, but do you need to see all registers at once? The screenshot looks really 
cluttered. Wouldn't a tab view be a good choice? Or show the registers below 
each other?

Furthermore, the screenshot show some issues in your .ui file. I suggest using 
group boxes instead of the labels. Also make sure to remove margins such that 
the borders can collapse which should already reduce the border-clutter.

> > 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#file183679l
> > > ine363>> > 
> > >     you don't need the _ prefix here
> 
> How then? tableWidget(tableWidget), name(name) {} ??

Yes.

Bye
-- 
Milian Wolff
mail at milianw.de
http://milianw.de