<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/122317/">https://git.reviewboard.kde.org/r/122317/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On Februar 15th, 2015, 5:46 nachm. UTC, <b>Alex Merry</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/122317/diff/2/?file=347076#file347076line131" style="color: black; font-weight: bold; text-decoration: underline;">modules/ECMGenerateHeaders.cmake</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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">131</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nb">if</span><span class="p">(</span><span class="s">NOT</span> <span class="s">EGH_ORIGINAL</span> <span class="s">STREQUAL</span> <span class="s2">"LOWERCASE"</span> <span class="s">AND</span> <span class="s">NOT</span> <span class="s">EGH_ORIGINAL</span> <span class="s">STREQUAL</span> <span class="s2">"CAMELCASE"</span><span class="p">)</span></pre></td>
</tr>
<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">132</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nb">message</span><span class="p">(</span><span class="s">FATAL_ERROR</span> <span class="s2">"Unexpected value for original argument to ECM_GENERATE_HEADERS: ${EGH_ORIGINAL}"</span><span class="p">)</span></pre></td>
</tr>
<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">133</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nb">endif</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;">Rather than artificially add yet another place to change when we introduce new variants, why not just make this part of the if/else statement in lines 148-152 below?</p></pre>
</blockquote>
<p>On Februar 15th, 2015, 8:21 nachm. UTC, <b>Friedrich W. H. Kossebau</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;">Because, if EGH_PREFIX is not set, then the lines in 148-152 will not be reached. But EGH_ORIGINAL must be always checked if a proper value is set.
So needs to be done outside that if/else.</p></pre>
</blockquote>
<p>On Februar 15th, 2015, 8:26 nachm. UTC, <b>Alex Merry</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;">Ah, you're right, my bad. It could go in the foreach, but there's certainly no need to check it every single iteration of the loop; OTOH, the maintainability of not having yet another thing to change with a new mode may outweigh the runtime cost of an extra string test in the loop.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'll leave it up to you whether you want to make that change, though, as I don't feel too strongly either way.</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;">K. Will leave it as it is as I prefer sanity checks to be at the begin, not "hidden" in the rest of the code (I need/prefer simple structures, with one place doing one thing, even if longer :) )
Also if the new variants come, the code might change differently from what we expect now (perhaps there could be a separate method generate_original_filename getting the EGH_ORIGINAL as paramter), so leaving code optimization to the person adding the variants and their needs.</p></pre>
<br />
<p>- Friedrich W. H.</p>
<br />
<p>On Februar 14th, 2015, 4:11 nachm. UTC, Friedrich W. H. Kossebau 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 Build System, Extra Cmake Modules and Alex Merry.</div>
<div>By Friedrich W. H. Kossebau.</div>
<p style="color: grey;"><i>Updated Feb. 14, 2015, 4:11 nachm.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
extra-cmake-modules
</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;">There are at least two projects I know which have CamelCase.h headers, Calligra and KDChart. Still it would be nice to also allow pure CamelCase headers (suffix less) for usage by lib-using developers. Just, currently the macro ecm_generate_headers assumes the original headers to be lowercase. So it cannot be used for the mentioned projects.
Worse, ecm_generate_headers also has a very nice additional feature, having copies of the original headers in the build dir created with the same layout as there will be after installation, so e.g. code of demos and tests can be written as if it is 3rd-party outside the project.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Attached patch extends ecm_generate_headers to support an optional parameter <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ORIGINAL</code> (perhaps a better term could be found), by which the casing of the original headers can be specified. Currently <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">CAMELCASE</code> and <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">LOWERCASE</code> are supported. But perhaps one day someone could extend it for underscore-casing :). The parameter defaults to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">LOWERCASE</code> and thus should be backwards portable.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Tested also with prefixes.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Not sure the assumption that the original prefix has the same casing as the original headers, I remember to have seen code where that is not consistent. But that step to implement is left for those persons who really need it ;)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Update:
See also discussion at https://git.reviewboard.kde.org/r/122193/ where so far opinions are positive on this one.</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;">Manually and in latest update also with 2 unit tests (hm, not sure if those tests should cover more combinations with other parameters).</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>modules/ECMGenerateHeaders.cmake <span style="color: grey">(bac5086)</span></li>
<li>tests/ECMGenerateHeadersTest/CamelCaseHeadTest1.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/ECMGenerateHeadersTest/CamelCaseHeadTest2.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/ECMGenerateHeadersTest/run_test.cmake.config <span style="color: grey">(0a2425f)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122317/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>