<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/115992/">https://git.reviewboard.kde.org/r/115992/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 1st, 2014, 5:04 p.m. UTC, <b>Milian Wolff</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/115992/diff/2/?file=251093#file251093line128" style="color: black; font-weight: bold; text-decoration: underline;">clangparsejob.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void visitInclusions(CXFile file, CXSourceLocation* stack, unsigned stackDepth, CXClientData d)</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">126</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">void</span> <span class="n">visitInclusions</span><span class="p">(</span><span class="n">CXFile</span> <span class="n">file</span><span class="p">,</span> <span class="n">CXSourceLocation</span><span class="o">*</span> <span class="n">stack</span><span class="p">,</span> <span class="kt">unsigned</span> <span class="n">stackDepth</span><span class="p">,</span> <span class="n">CXClientData</span> <span class="n">d</span><span class="p">)</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">128</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">static</span> <span class="k">const</span> <span class="n">QString</span> <span class="n">customIncludePathsFilename</span> <span class="o">=</span> <span class="n">QString</span><span class="o">::</span><span class="n">fromLatin1</span><span class="p">(</span><span class="s">".kdev_include_paths"</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;">FYI: there's also QLatin1String</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;">Sure, but then you still have to create a QString each time you use it, at least in principle.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 1st, 2014, 5:04 p.m. UTC, <b>Milian Wolff</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/115992/diff/2/?file=251095#file251095line48" style="color: black; font-weight: bold; text-decoration: underline;">duchain/clanghelpers.h</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">48</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">KDevelop</span><span class="o">::</span><span class="n">DeclarationPointer</span> <span class="n">findDeclaration</span><span class="p">(</span><span class="n">CXCursor</span> <span class="n">cursor</span><span class="p">,</span> <span class="k">const</span> <span class="n">IncludeFileContexts</span><span class="o">&</span> <span class="n">includes</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;">why are these two not exported?</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;">They aren't used outside of the library ..?</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 1st, 2014, 5:04 p.m. UTC, <b>Milian Wolff</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/115992/diff/2/?file=251098#file251098line48" style="color: black; font-weight: bold; text-decoration: underline;">duchain/clangpch.cpp</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">48</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">static</span> <span class="k">const</span> <span class="n">Path</span> <span class="n">ownPchInclude</span><span class="p">;</span> <span class="c1">//No chained pch for now</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;">what's this about? why is this static? constructing an empty path is cheap so you could leave it non-static?</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;">Documentary purposes (here is where you handle chained pch), but sure, meh.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 1st, 2014, 5:04 p.m. UTC, <b>Milian Wolff</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/115992/diff/2/?file=251100#file251100line58" style="color: black; font-weight: bold; text-decoration: underline;">duchain/clangtypes.cpp</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">58</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">pch</span> <span class="o">!=</span> <span class="n">m_pch</span><span class="p">.</span><span class="n">end</span><span class="p">()</span> <span class="o">&&</span> <span class="n">QFileInfo</span><span class="p">(</span><span class="n">pchInclude</span><span class="p">.</span><span class="n">toLocalFile</span><span class="p">()</span> <span class="o">+</span> <span class="n">pchExt</span><span class="p">).</span><span class="n">exists</span><span class="p">())</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;">the QFileInfo check could be done outside the QReadLocker, no? Actually even before the UrlParseLock? or do you think it might get deleted inbetween?
Generally, this can be simplified by doing this (which is also non-detaching btw):
if (QFile::exists(pchInclude.toLocalFile() + pchExt)) {
QReadLocker lock(&m_pchLock);
auto pch = m_pch.value(pchInclude);
if (pch) {
return pch;
}
}</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;">If the QFileInfo check is done before the UrlParseLock, multiple ClangParseJobs using the same TU may commit to creating the PCH after simultaneously discovering it doesn't exist. It shouldn't cause problems, as each creation will still be serialized, but it's not the correct behavior.</pre>
<br />
<p>- Olivier Jean de</p>
<br />
<p>On February 28th, 2014, 7:12 p.m. UTC, Olivier Jean de Gaalon 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 and Milian Wolff.</div>
<div>By Olivier Jean de Gaalon.</div>
<p style="color: grey;"><i>Updated Feb. 28, 2014, 7:12 p.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 little dirty, but I probably won't be able to work on it any more this week, so at least I can throw it up for feedback. Give special attention to parselock, because I didn't take the time to see how it's supposed to be used.</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;">It works, including with epic-size projects. It still doesn't resume properly for some reason (clang will complain about invalid PCH, create an invalid tu, and the pch contexts will be nuked). I haven't had time to look into that, just have to remove the pch and rebuild for now.</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>clangparsejob.h <span style="color: grey">(b12e9df)</span></li>
<li>clangparsejob.cpp <span style="color: grey">(86588ce)</span></li>
<li>duchain/CMakeLists.txt <span style="color: grey">(7ab2e8a)</span></li>
<li>duchain/clanghelpers.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>duchain/clanghelpers.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>duchain/clangpch.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>duchain/clangpch.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>duchain/clangtypes.h <span style="color: grey">(b4baba7)</span></li>
<li>duchain/clangtypes.cpp <span style="color: grey">(f8eb8b8)</span></li>
<li>duchain/includedfilecontexts.h <span style="color: grey">(4bdd6ca)</span></li>
<li>duchain/includedfilecontexts.cpp <span style="color: grey">(90d800c)</span></li>
<li>duchain/parsesession.h <span style="color: grey">(e557884)</span></li>
<li>duchain/parsesession.cpp <span style="color: grey">(265e099)</span></li>
<li>duchain/tuduchain.h <span style="color: grey">(e0e8d44)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/115992/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>