<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;">Can't test your patch, because it asserts early.
#5 0x00007ffff5e73cce in qt_assert (assertion=0x7ffff3d38ab8 "DUChain::lock()->currentThreadHasReadLock() || DUChain::lock()->currentThreadHasWriteLock()", file=0x7ffff3d38330 "/home/krf/devel/src/kdevp
latform/language/duchain/duchain.cpp", line=1312) at /home/krf/devel/src/qt/src/corelib/global/qglobal.cpp:2054
#6 0x00007ffff3c13a95 in KDevelop::DUChain::chainForDocument (this=this@entry=0x244cd90, document=..., proxyContext=proxyContext@entry=false) at /home/krf/devel/src/kdevplatform/language/duchain/duchain
.cpp:1312
#7 0x00007ffff3c13e57 in KDevelop::DUChain::chainForDocument (this=0x244cd90, document=..., proxyContext=<optimized out>) at /home/krf/devel/src/kdevplatform/language/duchain/duchain.cpp:1274
#8 0x00007ffff3c7e409 in KDevelop::DUChainUtils::standardContextForUrl (url=..., preferProxyContext=preferProxyContext@entry=false) at /home/krf/devel/src/kdevplatform/language/duchain/duchainutils.cpp:
277
#9 0x00007fff4be267ae in (anonymous namespace)::getSession (url=...) at /home/krf/devel/src/kdev-clang/codegen/clangsignatureassistant.cpp:56
#10 0x00007fff4be26e45 in ClangSignatureAssistant::textChanged (this=this@entry=0x41fa190, change=[(1, 7) -> (3, 4)]) at /home/krf/devel/src/kdev-clang/codegen/clangsignatureassistant.cpp:297
#11 0x00007fff4be23c4f in CodeAssistant::eventuallyStartAssistant (this=0x35c94d0) at /home/krf/devel/src/kdev-clang/codegen/codeassistant.cpp:214</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/3/?file=271269#file271269line66" style="color: black; font-weight: bold; text-decoration: underline;">codegen/clangsignatureassistant.h</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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">66</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">class</span> <span class="n">ClangSignatureAssistant</span> <span class="o">:</span> <span class="n">public</span> <span class="n">KDevelop</span><span class="o">::</span><span class="n">IAssistant</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 before {.
- Indent Q_OBJECT
</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/3/?file=271270#file271270line56" style="color: black; font-weight: bold; text-decoration: underline;">codegen/clangsignatureassistant.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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">56</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">auto</span> <span class="n">top</span> <span class="o">=</span> <span class="n">KDevelop</span><span class="o">::</span><span class="n">DUChainUtils</span><span class="o">::</span><span class="n">standardContextForUrl</span><span class="p">(</span><span class="n">url</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;">I suggest holding the DUChainReaderLocker here.
Just tested your patch and got an assert while accessing this function from within the 'textChanged' method.</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/3/?file=271270#file271270line82" style="color: black; font-weight: bold; text-decoration: underline;">codegen/clangsignatureassistant.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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">82</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">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">CXFile</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;">API change: return the CXCursor:
This will make the code simpler, too:
CXCursor cursor = ClangUtils::getCXCursor(sc.line, sc.column, unit, file));
if (clang_getCursorKind(cursor) == CXCursor_ParmDecl) {
cursor = clang_getCursorSemanticParent(cursor);
}
if (!CursorKindTraits::isFunction(clang_getCursorKind(cursor)))
return clang_getNullCursor();
return cursor;</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/3/?file=271270#file271270line382" style="color: black; font-weight: bold; text-decoration: underline;">codegen/clangsignatureassistant.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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">382</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">DUChainReadLocker</span> <span class="n">lock</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;">Do we need the lock here?</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/3/?file=271270#file271270line404" style="color: black; font-weight: bold; text-decoration: underline;">codegen/clangsignatureassistant.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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">404</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">CXFile</span> <span class="n">otherFile</span> <span class="o">=</span> <span class="n">clang_getFile</span><span class="p">(</span><span class="n">targetSession</span><span class="o">-></span><span class="n">unit</span><span class="p">(),</span> <span class="n">str</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;">Just use 'clang_getFile(..., m_otherLoc.document.byteArray().constData())'?</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/3/?file=271276#file271276line61" style="color: black; font-weight: bold; text-decoration: underline;">util/clangutils.h</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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">61</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">getCXCursor</span><span class="p">(</span><span class="kt">int</span> <span class="n">line</span><span class="p">,</span> <span class="kt">int</span> <span class="n">column</span><span class="p">,</span> <span class="n">CXTranslationUnit</span> <span class="n">unit</span><span class="p">,</span> <span class="n">CXFile</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;">Return the CXCursor</pre>
</div>
<br />
<p>- Kevin Funk</p>
<br />
<p>On May 3rd, 2014, 1:31 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 3, 2014, 1:31 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/clangsignatureassistant.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>codegen/clangsignatureassistant.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>codegen/codeassistant.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>codegen/codeassistant.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>duchain/clangtypes.h <span style="color: grey">(c7293a9)</span></li>
<li>duchain/clangtypes.cpp <span style="color: grey">(11760c8)</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>