Review Request 112039: Add a CPU registers view.

[Milian Wolff]

On Tuesday 20 August 2013 18:58:49 Vlas Puhov wrote:
> > 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#file183539l
> > > ine170>> > 
> > >     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.

But it's not required and smarter compilers might (eventually?) tell you so. 
Instead do this:

...toUInt(0, 16);

> > 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#file183539l
> > > ine152>> > 
> > >     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??

a) Consistency: A big code base needs rules otherwise maintenance is hell. 
Someone used to Qt/KDE codebases does not expect references but rather assumes 
the more explicit version with ptr's as in/out parameters. See e.g. the above 
toUInt code snippet.

b) lvalue/rvalue: 

foo& bar(foo& in) { return in; }

bar(foo(1));

this won't compile as a & cannot bind to an rvalue. Maybe you don't call it 
like that anywhere yet, but who knows what will happen in the future. Note 
that this also rules out the ptr-in/out argument. In most cases the const& + 
return copy is fine anyways.

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

This is an odd API anyways. Can you explain what you try to do here so we may 
think about a better API for this?

Also note how you pass the &ok to toUInt. This is simply the Qt-style.

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

Is this really being called zillion of times or only very rarely? Did you 
benchmark it and saw that its an issue? Anyhow, mutating objects will always 
lead to copies so you don't save anything with implicit sharing. You only save 
something if you pass them around without mutating them. Copying a 
RegistersGroup without mutating it only copies a struct with sizeof(24) on a 
64bit machine (I guess) which is pretty fast. Mutating a member then leads to 
a detach which may copy more data, depending on what is changed.

That being said, if you explain some more what you want to do (i.e. given A do 
B to reach state C) then we can think about a better API here.

> 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.:(

As I said above, references are less obvious as ptrs and the latter is the 
style we follow in Qt/KDE. That alone is reason enough not to use references.

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