<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/117836/">https://git.reviewboard.kde.org/r/117836/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 28th, 2014, 6:53 p.m. UTC, <b>Olivier Jean de Gaalon</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'd also like some clarification on the reasoning behind the patch, as you mention:
"Currently to retrieve i/d we use IBuildSystemManager, that's imo very strange, as I don't see any reason why e.g. builder() method and includes() should be in one interface? Language plugins obviously don't use the former..."
Language plugins aren't the only clients for IBuildSystemManager, builder() and includes() are in the same interface because they commonly share the same underlying data (provided by CMake, QMake, or any other buildsystem). That's IMO the best possible reason to decide what belongs in an interface. As to the custom buildsystem, I think it would make perfect sense to be able to manually define build targets as well as includes and defines, even if it doesn't right now.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">To clarify my take on this a bit, I certainly see the interface duplication between IADI and IBSM and agree that it should be accessible from one place. It also makes sense to me to allow adding custom includes and defines on top of any IBSM without each IBSM having to custom-implement that functionality (and multiple providers is a fine generalization of that requirement). I just want to understand better how you're positioning IADI.</pre>
<br />
<p>- Olivier Jean de</p>
<br />
<p>On April 28th, 2014, 11:04 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 April 28, 2014, 11:04 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevelop
</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;"> Currently to retrieve i/d we use IBuildSystemManager, that's imo very strange, as I don't see any reason why e.g. builder() method and includes() should be in one interface? Language plugins obviously don't use the former...
Also not all project manager use i/d functionality (e.g. Custom build system doesn't).
Finally, with that path applied it becomes simpler to retrieve i/d as now all information is accessible from one place.
There is one issue though: In CMakeManager::projectData() there is the assert with QThread::currentThread() == project->thread(), that I commented out. Currently it works without assertion, as processGeneratorExpression directly accesses m_projectsData, but with this path it becomes impossible to do that (only through friend class or something...).
So, is it ok to comment it out?</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;">Everything language/project related still passes (except cmakeprojectvisitortest, but it doesn't work for me either way.)</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>languages/cpp/includepathcomputer.cpp <span style="color: grey">(74fcc16)</span></li>
<li>languages/plugins/custom-definesandincludes/definesandincludesmanager.h <span style="color: grey">(bd560eb)</span></li>
<li>languages/plugins/custom-definesandincludes/definesandincludesmanager.cpp <span style="color: grey">(d1723d3)</span></li>
<li>languages/plugins/custom-definesandincludes/kdevdefinesandincludesmanager.desktop.cmake <span style="color: grey">(65b933f)</span></li>
<li>projectmanagers/cmake/cmakemanager.h <span style="color: grey">(19fc0c1)</span></li>
<li>projectmanagers/cmake/cmakemanager.cpp <span style="color: grey">(99e9e21)</span></li>
<li>projectmanagers/cmake/tests/cmakemanagertest.cpp <span style="color: grey">(4e83175)</span></li>
<li>projectmanagers/custom-buildsystem/custombuildsystemplugin.h <span style="color: grey">(0798602)</span></li>
<li>projectmanagers/custom-buildsystem/custombuildsystemplugin.cpp <span style="color: grey">(db194a4)</span></li>
<li>projectmanagers/custommake/custommakemanager.h <span style="color: grey">(6946e62)</span></li>
<li>projectmanagers/custommake/custommakemanager.cpp <span style="color: grey">(346c8cd)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/117836/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>