[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: Review Request 121831: libksysguard: process.h: encapsulate private fields
From: Thomas_Lübking <thomas.luebking () gmail ! com>
Date: 2015-01-26 17:52:27
Message-ID: 20150126175227.30683.1026 () probe ! kde ! org
[Download RAW message or body]
--===============2960422549339866551==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
> 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
>
> Dominik Haumann wrote:
> 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 ;)
> Thomas Lübking wrote:
> sorry - from the discussion i just assumed this was a leaf to qobject.
> no. introducing a vtable to a private class makes no sense and indeed does harm \
> (performance) and q_decl_override is of course entirely wrong in a base class.
>
> Dominik Haumann wrote:
> Oh, and I just recognized that this is about Process and not ProcessPrivate. My \
> previous comment was about ProcessPrivate, so pretty much off-topic.
> Ok, then I agree that it's ok to have a virtual destructor, since maybe someone \
> wants to inherit from this class... sorry for the noise :)
Hehe =)
Ok, looked up the patch in a real browser.
For an exported public root class, a virtual destructor is a very good idea (unless \
you *really* want to enforce this being a leaf) It then must have the virtual keyword \
(Q_DECL_OVERRIDE still makes no sense, that would be for the inheritors)
- Thomas
-----------------------------------------------------------
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
>
>
--===============2960422549339866551==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 8bit
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 \
solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> \
<tr> <td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/121831/">https://git.reviewboard.kde.org/r/121831/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On Januar 21st, 2015, 10:10 vorm. UTC, <b>Dominik \
Haumann</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;"> <a \
href="https://git.reviewboard.kde.org/r/121831/diff/3/?file=343193#file343193line40" \
style="color: black; font-weight: bold; text-decoration: \
underline;">processcore/process.h</a> <span style="font-weight: normal;">
(Diff revision 3)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th> <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td> <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">40</font></th> <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span \
class="n">virtual</span> <span class="o">~</span><span class="n">Process</span><span \
class="p">();</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Is \
virtual needed here?</p></pre> </blockquote>
<p>On Januar 25th, 2015, 9:52 nachm. UTC, <b>Gregor Mi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Does \
it hurt here to have it virtual? I thought, if in doubt 'virtual' should be \
used.</p></pre> </blockquote>
<p>On Januar 26th, 2015, 11:35 vorm. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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</p></pre>
</blockquote>
<p>On Januar 26th, 2015, 11:55 vorm. UTC, <b>Dominik Haumann</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">How \
is Q_DECL_OVERRIDE related to having a virtual destructor in the base class?</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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...</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">I dont think we need a virtual \
destructor here. only adds a vtable that is not required ;)</p></pre> </blockquote>
<p>On Januar 26th, 2015, 12:24 nachm. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">sorry \
- from the discussion i just assumed this was a leaf to qobject. no. introducing a \
vtable to a private class makes no sense and indeed does harm (performance) and \
q_decl_override is of course entirely wrong in a base class.</p></pre> </blockquote>
<p>On Januar 26th, 2015, 5:06 nachm. UTC, <b>Dominik Haumann</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Oh, \
and I just recognized that this is about Process and not ProcessPrivate. My previous \
comment was about ProcessPrivate, so pretty much off-topic.</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ok, \
then I agree that it's ok to have a virtual destructor, since maybe someone wants to \
inherit from this class... sorry for the noise :)</p></pre> </blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Hehe =)</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">Ok, looked up the patch in a real \
browser.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">For an exported public root class, a virtual \
destructor is a very good idea (unless you <em style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: normal;">really</em> want to \
enforce this being a leaf) It then must have the virtual keyword (Q_DECL_OVERRIDE \
still makes no sense, that would be for the inheritors)</p></pre> <br />
<p>- Thomas</p>
<br />
<p>On Januar 25th, 2015, 10:27 nachm. UTC, Gregor Mi wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: \
1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; \
-webkit-border-radius: 6px;"> <tr>
<td>
<div>Review request for KDE Base Apps, Dominik Haumann, Eike Hein, and John \
Tapsell.</div> <div>By Gregor Mi.</div>
<p style="color: grey;"><i>Updated Jan. 25, 2015, 10:27 nachm.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
libksysguard
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">In process.h there are several public fields. This RR \
introduces a d-ptr.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">(In a separate commit I would add the \
.reviewboardrc file)</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">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)?</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Compiles and runs. Data is still shown; no visible \
error. Unit tests succeed.</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>processui/scripting.h <span style="color: \
grey">(2445c0ab0d81af3283c0f6e9c5f349a3d70b0de9)</span></li>
<li>processui/scripting.cpp <span style="color: \
grey">(76291b0ae0a26e486aa81a4ca3976ff4a47cb3c0)</span></li>
<li>processcore/processes_solaris_p.cpp <span style="color: \
grey">(f054df4b1e762e9cbec1ff8dea78f467b878bee0)</span></li>
<li>processui/ProcessFilter.cpp <span style="color: \
grey">(ec520593fb67c777d56817f2493d40dc5ade0347)</span></li>
<li>processui/ProcessModel.cpp <span style="color: \
grey">(53bc041110c9cdb686fef783895104969b661889)</span></li>
<li>processui/ksysguardprocesslist.cpp <span style="color: \
grey">(4dc142e864d8353ceafc3a6735ffa81e48291420)</span></li>
<li>processcore/processes.cpp <span style="color: \
grey">(580df8db152040f1ad075430fdce08fe7ad4ae2d)</span></li>
<li>processcore/processes_atop_p.cpp <span style="color: \
grey">(24c76e3e35f62bd8e9e705ad32cc11cbd3662601)</span></li>
<li>processcore/processes_base_p.h <span style="color: \
grey">(71b8a9cc6ee14bf7934a0a9d3199b257b5ce1be7)</span></li>
<li>processcore/processes_linux_p.cpp <span style="color: \
grey">(898d4fa491873fe95a8b32a5c1b85642b2e46ad5)</span></li>
<li>processcore/processes.h <span style="color: \
grey">(d09c3265333fe7e2702deaa910c5fbe4bc3ac9e6)</span></li>
<li>tests/processtest.cpp <span style="color: \
grey">(f9b36e9a3a3c2048b51f1d935f8c40de2ad8a9b8)</span></li>
<li>.reviewboardrc <span style="color: grey">(PRE-CREATION)</span></li>
<li>CMakeLists.txt <span style="color: \
grey">(cefc86f12be684e195bd148641483e9e1734e636)</span></li>
<li>processcore/process.h <span style="color: \
grey">(b6695c0ed301dc5f0fad8ba847da811f19ebfd9a)</span></li>
<li>processcore/process.cpp <span style="color: \
grey">(a38b8be71da1a51cb87f636664ebac817b1d20ab)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121831/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============2960422549339866551==--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic