<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/119502/">https://git.reviewboard.kde.org/r/119502/</a>
</td>
</tr>
</table>
<br />
<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;">Hm. Patch looks okay to me functionality-wise. But style-wise most unsets jump into my eye. Perhaps I am just too used to "set(FOO)". But it also seems unset is simply not that typical in cmake files in KDE software, see http://lxr.kde.org/search?_filestring=%28%5C.cmake%24%29%7CCMakeLists.txt&_advanced=1&_string=unset<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
The pattern there seems to be to use unset() rather to clean up at the end, less to initialize variables at the begin. Which somehow also reflects my direct idea of the semantics by the word (even if functional they are identic, as far as I got the manual). Having a var unset at the beginning surprises me more than setting it to nothing.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The other thing that catches my eyes is that using unset for any cases where a var should be empty/not set now means that initialization of helper vars at begin of a macro is a mixture of set and unset, resulting in no-longer vertically aligned variable names and thus harder to get the list of vars.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
See e.g. the macro calligra_define_product:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
set(_product_name "${_product_id}")<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
unset(_needed_dep_product_ids)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So having the usage of unset actually in front of my eyes I am undecided if deriving from the more common pattern in KDE and having vars not aligned is really an advantage, sorry.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What do others think?</p></pre>
<br />
<p>- Friedrich W. H. Kossebau</p>
<br />
<p>On Juli 27th, 2014, 5:49 nachm. UTC, Elvis Stansvik 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 Calligra and Friedrich W. H. Kossebau.</div>
<div>By Elvis Stansvik.</div>
<p style="color: grey;"><i>Updated Juli 27, 2014, 5:49 nachm.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
calligra
</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;">It's slightly more clear to non-CMake wizards to use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">unset(MY_VAR)</code> instead of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">set(MY_VAR)</code> to unset a variable. I found such places with the slightly ugly command</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">find . <span style="border: 1px solid #FF0000">\</span>( <span style="color: #666666">-</span>name <span style="color: #BA2121">"*.cmake"</span> <span style="color: #666666">-</span>o <span style="color: #666666">-</span>name <span style="color: #BA2121">"CMakeLists.txt"</span> <span style="border: 1px solid #FF0000">\</span>) <span style="color: #666666">-</span>exec egrep <span style="color: #666666">-</span>Hn <span style="color: #BA2121">"(^|\s+)[sS][eE][tT][ ]*\([ ]*[^ ]+[ ]*\)"</span> {} <span style="border: 1px solid #FF0000">\</span>;
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">and changed them to use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">unset(..)</code> instead.</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;">Did a default CMake run. Everything seemed fine, the semantics should not have changed.</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>cmake/modules/FindICU.cmake <span style="color: grey">(46671c8)</span></li>
<li>cmake/modules/FindIconv.cmake <span style="color: grey">(ce40ab2)</span></li>
<li>cmake/modules/FindPoppler.cmake <span style="color: grey">(534acbc)</span></li>
<li>cmake/modules/MacroCalligraAddBenchmark.cmake <span style="color: grey">(2178adf)</span></li>
<li>krita/CMakeLists.txt <span style="color: grey">(3668a56)</span></li>
<li>krita/benchmarks/CMakeLists.txt <span style="color: grey">(86794a5)</span></li>
<li>libs/pigment/CMakeLists.txt <span style="color: grey">(fb1a54f)</span></li>
<li>plugins/colorengines/lcms2/CMakeLists.txt <span style="color: grey">(ae4d140)</span></li>
<li>cmake/modules/CalligraProductSetMacros.cmake <span style="color: grey">(8b0492b)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119502/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>