<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>
<p>On April 28th, 2014, 7:09 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;">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>
</blockquote>
<p>On April 29th, 2014, 11:26 a.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 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).
That's exactly why I did it like this in the first place!
> I just want to understand better how you're positioning IADI
Sorry, I don't get it, what do you mean by "how you're positioning IADI"?</pre>
</blockquote>
<p>On April 29th, 2014, 11:59 a.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;">> That's exactly why I did it like this in the first place!
Yes, and I am agreeing with your implementation. Patch is good :).
My only question was about your description/explanation of the patch: I just see this as a cleanup of duplication, but it sounded like you wanted to split up IBSM into various other interfaces. I see that's not actually the case, and your point was just that the CustomBuildSystemManager now doesn't need to be a provider since that component was generalized. I guess that makes sense, though it's somewhat of a special case... every other buildsystem is going to also be a IDefinesAndIncludesManager::Provider.</pre>
</blockquote>
<p>On April 29th, 2014, 4:08 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;">So here the reasons why I think it's a good idea to extend IADI:
1. It removes interface duplication between IADI and IBSM
2. Not all project managers use i/d functionality
3. It use to be easy to query for i/d through IBSM, but currently there are already 2 places to get that information (IADM and IBSM). Also I'm planning to make compiler specific (the ones that we query compiler for) i/d editable too, so in future there'll be at least three different sources of i/d, and I see no easy way how IBSM in current state could handle that...
So that's why I'm suggesting to implement new interface where languages plugins and project managers can register providers that provide i/d, which afterwards can be edited by user. And finally can be retrieved by language plugins.
If my current approach seems inappropriate for you, please suggest how you'd do it, because I'm honestly out of ideas here...
</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;">Like I said, the approach is good, thanks for clarifying the description though. Let's get this threading issue worked out and ship it.</pre>
<br />
<p>- Olivier Jean de</p>
<br />
<p>On April 29th, 2014, 4:04 p.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 29, 2014, 4:04 p.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.</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>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<ul>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/04/29/f68a81a0-292b-4808-a3ab-5fa7f1348b7e__backtreces.tar.gz">backtreces.tar.gz</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>