<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/117922/">https://git.reviewboard.kde.org/r/117922/</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;">As others already mentioned, StaticCodeAssisant (which you've now named ClangCodeAssistant) will be moved to kdevplatform, so other languages can benefit from it, too. I have a local branch in kdevplatform, where I have a cleaned up version of StaticCodeAssistant already. There's some more work needed in order to make it accessible from language plugins, which I'm planning to do in my first week of GSoC (ie. in ~2 weeks).
So, in other words. ClangCodeAssistant is very likely going to be dropped soonish. Like Milian said, let's ignore that for now and make sure your patch gets in either way.
Just some general remarks:
- I'd suggest not putting any more effort into porting stuff which uses or is invoked by StaticCodeAssistant (as I already have intrusive local changes which I didn't upstream yet)
- ClangSignatureAssistant still makes sense, it won't go away.
- And as others have said: Please make sure to add some tests as well (feel free to do that in a follow-up commit, but please do it) :)
</pre>
<br />
<div>
<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/117922/diff/1/?file=270532#file270532line57" style="color: black; font-weight: bold; text-decoration: underline;">codegen/clangsignatureassistant.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">57</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">bool</span> <span class="nf">getSession</span><span class="p">(</span><span class="n">KUrl</span> <span class="n">url</span><span class="p">,</span> <span class="n">KSharedPtr</span><span class="o"><</span><span class="n">ParseSession</span><span class="o">></span> <span class="o">&</span><span class="n">session</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">That's a bit overcomplicated, IMO, and not really the C++-way. Just return the session-ptr. The caller needs to check against nullptr then.
Also: const& 'url'</pre>
</div>
<br />
<div>
<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/117922/diff/1/?file=270532#file270532line86" style="color: black; font-weight: bold; text-decoration: underline;">codegen/clangsignatureassistant.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">86</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">template</span><span class="o"><</span><span class="k">typename</span> <span class="n">T</span><span class="o">></span> <span class="kt">bool</span> <span class="n">getFunctionCursor</span><span class="p">(</span><span class="k">const</span> <span class="n">SimpleCursor</span> <span class="o">&</span><span class="n">sc</span><span class="p">,</span> <span class="k">const</span> <span class="n">CXTranslationUnit</span> <span class="o">&</span><span class="n">unit</span><span class="p">,</span> <span class="k">const</span> <span class="n">T</span> <span class="o">&</span><span class="n">file</span><span class="p">,</span> <span class="n">CXCursor</span> <span class="o">&</span><span class="n">cursor</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Nitpick: Newline after template<></pre>
</div>
<br />
<div>
<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/117922/diff/1/?file=270532#file270532line99" style="color: black; font-weight: bold; text-decoration: underline;">codegen/clangsignatureassistant.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">99</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">ClangAdaptSignatureAction</span><span class="o">::</span><span class="n">ClangAdaptSignatureAction</span><span class="p">(</span><span class="kt">bool</span> <span class="n">targetDecl</span><span class="p">,</span> <span class="n">KUrl</span><span class="o">&</span> <span class="n">url</span><span class="p">,</span> <span class="n">KDevelop</span><span class="o">::</span><span class="n">SimpleRange</span><span class="o">&</span> <span class="n">range</span><span class="p">,</span> <span class="n">QString</span><span class="o">&</span> <span class="n">newSig</span><span class="p">,</span> <span class="n">QString</span><span class="o">&</span> <span class="n">oldSig</span><span class="p">)</span><span class="o">:</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">const& for params?</pre>
</div>
<br />
<div>
<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/117922/diff/1/?file=270532#file270532line205" style="color: black; font-weight: bold; text-decoration: underline;">codegen/clangsignatureassistant.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">205</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">clang_equalCursors</span><span class="p">(</span><span class="n">clang_getNullCursor</span><span class="p">(),</span> <span class="p">(</span><span class="n">otherSide</span> <span class="o">=</span> <span class="n">clang_getCursorDefinition</span><span class="p">(</span><span class="n">altCursor</span><span class="p">))))</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This assignment here ('otherSide =') in the if-condition is *very* hideous. Can you re-write that?</pre>
</div>
<br />
<div>
<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/117922/diff/1/?file=270532#file270532line234" style="color: black; font-weight: bold; text-decoration: underline;">codegen/clangsignatureassistant.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">234</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_otherFile</span> <span class="o">=</span> <span class="k">new</span> <span class="n">ClangString</span><span class="p">(</span><span class="n">clang_getFileName</span><span class="p">(</span><span class="n">f</span><span class="p">));</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Please use ClangLocation here and turn it into a DocumentCursor.
Note: I've just pushed some changes related to ClangLocation that makes it work with DocumentCursor.</pre>
</div>
<br />
<div>
<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/117922/diff/1/?file=270532#file270532line247" style="color: black; font-weight: bold; text-decoration: underline;">codegen/clangsignatureassistant.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">247</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">bool</span> <span class="n">findDeclEnd</span><span class="p">(</span><span class="n">KTextEditor</span><span class="o">::</span><span class="n">Document</span> <span class="o">*</span><span class="n">targetDoc</span><span class="p">,</span> <span class="n">CXCursor</span> <span class="n">cursor</span><span class="p">,</span> <span class="n">SimpleCursor</span> <span class="o">&</span><span class="n">sc</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Again, not really the C++ way. Why not just returning SimpleCursor?</pre>
</div>
<br />
<div>
<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/117922/diff/1/?file=270534#file270534line88" style="color: black; font-weight: bold; text-decoration: underline;">util/clangutils.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">namespace ClangUtils</pre></td>
</tr>
</tbody>
<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">88</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> */</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Please add a TODO stating that we should get rid off this as soon as we depend on Clang 3.5</pre>
</div>
<br />
<div>
<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/117922/diff/1/?file=270535#file270535line61" style="color: black; font-weight: bold; text-decoration: underline;">util/clangutils.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">61</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Remove extra whitespace.</pre>
</div>
<br />
<p>- Kevin Funk</p>
<br />
<p>On May 1st, 2014, 5:08 a.m. UTC, David Stevens 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 David Stevens.</div>
<p style="color: grey;"><i>Updated May 1, 2014, 5:08 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdev-clang
</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;">This is a preliminary implementation of the adjust signature helper. It works fairly well under normal usage, but it runs into problems if the user starts trying to use it at a high frequency. There are some rather fundamental issues both due to the different information organization (files vs translation units) and due to the discrepencies between what KDevelop has in the text editor and what clang has in the translation units. Hopefully I can get some feedback on the implementation.</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;">Manual. Any unit tests would be deeply tied into the GUI, so I'm not sure how to go about writing those. The old c++ plugin unfortunately doesn't have any tests to work off of.</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>clangsupport.h <span style="color: grey">(1316f88)</span></li>
<li>clangsupport.cpp <span style="color: grey">(5e3f464)</span></li>
<li>codecompletion/completionhelper.cpp <span style="color: grey">(dfaf6b3)</span></li>
<li>codegen/CMakeLists.txt <span style="color: grey">(0b913bd)</span></li>
<li>codegen/clangcodeassistant.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>codegen/clangcodeassistant.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>codegen/clangsignatureassistant.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>codegen/clangsignatureassistant.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/CMakeLists.txt <span style="color: grey">(c2fd36d)</span></li>
<li>util/clangutils.h <span style="color: grey">(3516821)</span></li>
<li>util/clangutils.cpp <span style="color: grey">(a74017c)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/117922/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>