<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/117835/">https://git.reviewboard.kde.org/r/117835/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 2nd, 2014, 1:19 p.m. UTC, <b>Milian Wolff</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/117835/diff/2/?file=270613#file270613line66" style="color: black; font-weight: bold; text-decoration: underline;">project/interfaces/ibuildsystemmanager.h</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">class ProjectTargetItem;</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">64</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">virtual</span> <span class="n">Path</span><span class="o">::</span><span class="n">List</span> <span class="n">includeDirectories</span><span class="p">(</span><span class="n">ProjectBaseItem</span><span class="o">*</span><span class="p">)</span> <span class="k">const</span><span class="hl"> </span><span class="o"><span class="hl">=</span></span><span class="hl"> </span><span class="mi"><span class="hl">0</span></span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">59</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">virtual</span> <span class="n">Path</span><span class="o">::</span><span class="n">List</span> <span class="n">includeDirectories</span><span class="p">(</span><span class="n">ProjectBaseItem</span><span class="o">*</span><span class="p">)</span> <span class="k">const</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;">because it is an interface</pre>
</blockquote>
<p>On May 2nd, 2014, 2:28 p.m. UTC, <b>Sergey Kalinichev</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;">I don't get it. I've just went through KDevelop's code base and it seems like pure interfaces are very rare, most provide default implementation for some functions, I see no reason why IBSM can't do it that way too, especially when we already have 2 plugins that don't use that functionality.</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;">This is getting into serious bikeshedding territory, as you're right that it's not a big deal. Still, I'm with Milian on the color we're painting with, maybe for different reasons.
CustomMakeBuildsystem doesn't provide include paths because it's deficient, CustomBuildSystem is the exception here. In fact, I could see an argument for KDevelop to replace the CustomBuildSystem with parallel systems like you've created for the includes and defines. This would eventually allow one to override or enhance any aspect of the buildsystem plugin in use, or not use a buildsystem plugin at all.
It's a matter of perspective. Mine is that your changes provide useful customization features but they don't change the fact that it's the IBuildSystemManager's job to provide includes and defines. I wouldn't want to mainstream any buildsystem plugin that didn't implement these functions and depended on the user providing them. The correct way to encode that is with pure virtual.
Given that, ship it (from me)!</pre>
<br />
<p>- Olivier Jean de</p>
<br />
<p>On May 1st, 2014, 11:20 a.m. UTC, Sergey Kalinichev wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Sergey Kalinichev.</div>
<p style="color: grey;"><i>Updated May 1, 2014, 11:20 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevplatform
</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;">The changes include extending IDM interface and removing i/d notion from IBuildSystemManager interface.</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>language/interfaces/idefinesandincludesmanager.h <span style="color: grey">(ab42444)</span></li>
<li>project/interfaces/ibuildsystemmanager.h <span style="color: grey">(241b696)</span></li>
<li>project/interfaces/ibuildsystemmanager.cpp <span style="color: grey">(74af0e7)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/117835/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>