Review Request 121831: libksysguard: process.h: encapsulate private fields

[Dominik Haumann]

> On Jan. 21, 2015, 10:10 vorm., Dominik Haumann wrote:
> > processcore/process.h, line 40
> > <https://git.reviewboard.kde.org/r/121831/diff/3/?file=343193#file343193line40>
> >
> >     Is virtual needed here?
> 
> Gregor Mi wrote:
>     Does it hurt here to have it virtual? I thought, if in doubt 'virtual' should be used.
> 
> Thomas Lübking wrote:
>     No. But it's not required either.
>     It's a matter of preference to indicate the virtuality in a non-root class.
>     The better solution is Q_DECL_OVERRIDE

How is Q_DECL_OVERRIDE related to having a virtual destructor in the base class?

Typically, you make the pimpl class FooPrivate virtual if there are other classes Bar that derive from Foo and also require the pimpl idion, i.e. class BarPrivate : public FooPrivate. This way you still have only one d-Pointer allocation independent of the deepness of the class hierarchy. In KDE, we seldom need that, and therefore we can put FooPrivate into the cpp file and make it have a non-virtual destructor.
However, in Qt itselv the d-pointer classes inherit other d-pointer classes, therefore you typically have a e.g. a qpushbutton_p.h, which itself probably includes qwidget_p.h etc...

I dont think we need a virtual destructor here. only adds a vtable that is not required ;)


- Dominik


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121831/#review74463
-----------------------------------------------------------


On Jan. 25, 2015, 10:27 nachm., Gregor Mi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121831/
> -----------------------------------------------------------
> 
> (Updated Jan. 25, 2015, 10:27 nachm.)
> 
> 
> Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John Tapsell.
> 
> 
> Repository: libksysguard
> 
> 
> Description
> -------
> 
> In process.h there are several public fields. This RR introduces a d-ptr.
> 
> 
> (In a separate commit I would add the .reviewboardrc file)
> 
> What is the current policy on using small C++ macros as done in this RR? Use it (code is more compact and readable) or don't use it (better for debugging)?
> 
> 
> Diffs
> -----
> 
>   processui/scripting.h 2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9 
>   processui/scripting.cpp 76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0 
>   processcore/processes_solaris_p.cpp f054df4b1e762e9cbec1ff8dea78f467b878bee0 
>   processui/ProcessFilter.cpp ec520593fb67c777d56817f2493d40dc5ade0347 
>   processui/ProcessModel.cpp 53bc041110c9cdb686fef783895104969b661889 
>   processui/ksysguardprocesslist.cpp 4dc142e864d8353ceafc3a6735ffa81e48291420 
>   processcore/processes.cpp 580df8db152040f1ad075430fdce08fe7ad4ae2d 
>   processcore/processes_atop_p.cpp 24c76e3e35f62bd8e9e705ad32cc11cbd3662601 
>   processcore/processes_base_p.h 71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7 
>   processcore/processes_linux_p.cpp 898d4fa491873fe95a8b32a5c1b85642b2e46ad5 
>   processcore/processes.h d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6 
>   tests/processtest.cpp f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8 
>   .reviewboardrc PRE-CREATION 
>   CMakeLists.txt cefc86f12be684e195bd148641483e9e1734e636 
>   processcore/process.h b6695c0ed301dc5f0fad8ba847da811f19ebfd9a 
>   processcore/process.cpp a38b8be71da1a51cb87f636664ebac817b1d20ab 
> 
> Diff: https://git.reviewboard.kde.org/r/121831/diff/
> 
> 
> Testing
> -------
> 
> Compiles and runs. Data is still shown; no visible error. Unit tests succeed.
> 
> 
> Thanks,
> 
> Gregor Mi
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20150126/faa630c6/attachment.htm>