<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="http://git.reviewboard.kde.org/r/104777/">http://git.reviewboard.kde.org/r/104777/</a>
</td>
</tr>
</table>
<br />
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Ben Wagner.</div>
<p style="color: grey;"><i>Updated May 1, 2012, 3:30 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Changes</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;">This patch fixes some of the messy and adds a unit test. There is one caveat however, and this may be opening a can of worms. In the test there are two ProjectBaseItem for the main.cpp url. The one with the target as the parent works as expected with this change, the 'include' include directory is visible. The one with the folder as the parent does not see the 'include' include directory, as would be expected from the implementation. It appears that the parser and the like always see the target parented version, as when I look at the new 'target_includes' test project the 'include' include directory certainly is found. The unit test itself has some extra debugging code in it which may or may not be wanted.
After thinking about this for a bit, I realized that there isn't really a good way around this. It is possible that two different targets will list the same source file. Because the includes for the targets can now be different these two mentions of the same source file may have entirely different meanings. Before, if the same source file was mentioned in two CMakeLists.txt files, the file would at least be out-of-source in at least one of them. I realized that defines have the same issue, so I took a look there. It is possible to set COMPILE_DEFINITIONS on directories, targets, and source files (though I don't think KDevelop handles source files right now). It appears to have the same issue, so I suspect that this is ok.
Also, now that I look at this patch with my reviewer hat on, I wonder if CMakeManager::includeDirectories should be implemented as I have here, or if it should be more like CMakeManager::defines where it finds the first 'DefinesAttached' thing and then just asks it to resolve. Now that I stare at it, I'm not sure my new code or the old code handles nested sub-directories with includes correctly (it appears to stop at the first folder found). Perhaps this should just go in as-is with another patch for nested sub-directories, if that is needed.</pre>
</td>
</tr>
</table>
<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;">With CMake 2.8.8, it is now possible to write something like
set_property(TARGET zlib APPEND PROPERTY INCLUDE_DIRECTORIES "include/config")
This change implements include directories on targets.
Note that I am not a committer, so if you want it, the diff I uploaded is a format-patch against current master.</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;">Built and ran. The test linked from bug works as expected after building (as the test itself generates some of the required .h files). Behavior of non-target includes appears unaffected.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=298859">298859</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> (updated)</h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>projectmanagers/cmake/cmakemanager.cpp <span style="color: grey">(9ae8f90)</span></li>
<li>projectmanagers/cmake/cmakemodelitems.h <span style="color: grey">(705a632)</span></li>
<li>projectmanagers/cmake/cmakemodelitems.cpp <span style="color: grey">(957c410)</span></li>
<li>projectmanagers/cmake/tests/CMakeLists.txt <span style="color: grey">(ca8567e)</span></li>
<li>projectmanagers/cmake/tests/cmakemanagertest.h <span style="color: grey">(1316952)</span></li>
<li>projectmanagers/cmake/tests/cmakemanagertest.cpp <span style="color: grey">(ca3532d)</span></li>
<li>projectmanagers/cmake/tests/manual/target_includes/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>projectmanagers/cmake/tests/manual/target_includes/includes/target_include.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>projectmanagers/cmake/tests/manual/target_includes/main.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>projectmanagers/cmake/tests/manual/target_includes/target_includes.kdev4 <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104777/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>