<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/121757/">https://git.reviewboard.kde.org/r/121757/</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;">Hey Olivier, nice to see you working on kdev-clang again :] You are missing the sources for <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">tumapping.*</code>. But from what I understand, you'll have a mutex-guarded QHash or such in there that maps any IndexedString to another one that represents the TU. That would be the last .cpp that included a given header, correct? Meaning, whenever a .cpp is parsed, all the headers it includes (i.e. most of STL and Qt) are continously re-pinned in the mapping? Or would it only be pinned once?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What would happen if a header with insufficient defines/include paths is parsed, it would again repin the headers it includes, right? So esp. for local includes that could be bad:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">foo.h (includes bar.h)
bar.h (includes some Qt header or such)
bar.cpp (includes bar.h)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">-> if foo.h is parsed after bar.h and bar.cpp and no include paths are found for foo.h, then bar.h would be found (local lookup), but the Qt headers are not found. Then bar.h is "updated" from this problematic include set and pinned to foo.h? Or is there some safety in the code that I missed which would prevent that from happening, and bar.h would still be pinned to bar.cpp?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also, I wonder why you pass the TU url to the Clang API, as mentioned below - please explain this a bit more to me.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Unrelated to the issues I point out, I agree that this approach is far more sophisticated than my simple approach with the buddy system. I also agree that we could potentially merge the two, such that the buddy is still requested for include paths and defines when we did not find a TU mapping.</p></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/121757/diff/2/?file=337766#file337766line58" style="color: black; font-weight: bold; text-decoration: underline;">duchain/clangparsingenvironmentfile.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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">using namespace KDevelop;</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">55</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ClangParsingEnvironment</span><span class="o">::</span><span class="n">EnvironmentQuality</span> <span class="n">lastQuality</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">rename this to quality</p></pre>
</div>
</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/121757/diff/2/?file=337769#file337769line111" style="color: black; font-weight: bold; text-decoration: underline;">duchain/parsesession.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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void addIncludes(QVector<const char*>* args, QVector<QByteArray>* otherArgs,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">111</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">ParseSessionData</span><span class="o">::</span><span class="n">ParseSessionData</span><span class="p">(</span><span class="k">const</span> <span class="n">IndexedString</span><span class="o">&</span> <span class="n">url</span><span class="p">,</span> <span class="k">const</span> <span class="n">QByteArray</span><span class="o">&</span> <span class="n"><span class="hl">c</span>ontents</span><span class="p">,</span> <span class="n">ClangIndex</span><span class="o">*</span> <span class="n">index</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">111</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">ParseSessionData</span><span class="o">::</span><span class="n">ParseSessionData</span><span class="p">(</span><span class="k">const</span> <span class="n">IndexedString</span><span class="o">&</span> <span class="n">url</span><span class="p">,</span> <span class="k">const</span> <span class="n"><span class="hl">IndexedString</span></span><span class="o"><span class="hl">&</span></span><span class="hl"> </span><span class="n"><span class="hl">modUrl</span></span><span class="p"><span class="hl">,</span></span><span class="hl"> </span><span class="k"><span class="hl">const</span></span><span class="hl"> </span><span class="n">QByteArray</span><span class="o">&</span> <span class="n"><span class="hl">modC</span>ontents</span><span class="p">,</span> <span class="n">ClangIndex</span><span class="o">*</span> <span class="n">index</span><span class="p">,</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">mod? I don't like the prefix name. maybe rather rename url to tuUrl here and keep the old url + contents?</p></pre>
</div>
</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/121757/diff/2/?file=337769#file337769line172" style="color: black; font-weight: bold; text-decoration: underline;">duchain/parsesession.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; ">ParseSessionData::ParseSessionData(const IndexedString& url, const QByteArray& contents, ClangIndex* index,</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">ParseSessionData::ParseSessionData(const IndexedString& url, const IndexedString& modUrl, const QByteArray& modContents, ClangIndex* index,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">172</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">index</span><span class="o">-></span><span class="n">index</span><span class="p">(),</span> <span class="n"><span class="hl">file</span></span><span class="p"><span class="hl">.</span></span><span class="n"><span class="hl">Filename</span></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">172</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">index</span><span class="o">-></span><span class="n">index</span><span class="p">(),</span> <span class="n"><span class="hl">url</span></span><span class="p"><span class="hl">.</span></span><span class="n"><span class="hl">byteArray</span></span><span class="p"><span class="hl">()</span>,</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">hmmm I wonder what effect that has on clang. why do you pass the TU url here? wouldn't that break the caching in clang, i.e. when a file is reparsed? can you comment some more on your design decision here?</p></pre>
</div>
</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/121757/diff/2/?file=337771#file337771line601" style="color: black; font-weight: bold; text-decoration: underline;">tests/test_duchain.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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void TestDUChain::testParsingEnvironment()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">601</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">env</span><span class="p">.</span><span class="n">setProjectKnown</span><span class="p">(</span><span class="nb">true</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">601</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">env</span><span class="p">.</span><span class="n">setQuality</span><span class="p">(</span><span class="n">ClangParsingEnvironment</span><span class="o">::</span><span class="n">Target</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">there is no test for ::Project, is there?</p></pre>
</div>
</div>
<br />
<p>- Milian Wolff</p>
<br />
<p>On January 2nd, 2015, 1:51 p.m. UTC, Olivier Jean de Gaalon 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 KDevelop, Kevin Funk and Milian Wolff.</div>
<div>By Olivier Jean de Gaalon.</div>
<p style="color: grey;"><i>Updated Jan. 2, 2015, 1:51 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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Note: This requires IBuildSystemManager::hasIncludesOrDefines, which should probably be renamed to isTranslationUnit()</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The idea here is to use the AST of the buildsystem's translation units wherever possible, since that's the only file that's guaranteed to have correct defines and includes (and furthermore can handle strange cases such as '#include inside namespace' and picky include/define ordering).
This proposal essentially allows TU environments to override non-TU environments and then get "pinned" so that the non-TU will continue to use the TU AST to build its duchain.
This could eventually be exposed in the UI, allowing the user to pick different "views" of headers (overriding the pinned TU).
The pinning data needs to be fleshed out to handle expiry (header no longer included) and be serialized with an ItemRepository.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm looking for feedback on this idea... where does it break? Any better ideas?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This more or less works, we can flesh it out more if we want to take this route.</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;">Tests fail in the same manner as before</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>clangparsejob.h <span style="color: grey">(1cc8282)</span></li>
<li>clangparsejob.cpp <span style="color: grey">(297b836)</span></li>
<li>duchain/CMakeLists.txt <span style="color: grey">(72464a5)</span></li>
<li>duchain/clanghelpers.cpp <span style="color: grey">(5f29a2a)</span></li>
<li>duchain/clangparsingenvironment.h <span style="color: grey">(7bb8111)</span></li>
<li>duchain/clangparsingenvironment.cpp <span style="color: grey">(1decc14)</span></li>
<li>duchain/clangparsingenvironmentfile.h <span style="color: grey">(953ee94)</span></li>
<li>duchain/clangparsingenvironmentfile.cpp <span style="color: grey">(b3d0563)</span></li>
<li>duchain/clangpch.cpp <span style="color: grey">(1ce3457)</span></li>
<li>duchain/parsesession.h <span style="color: grey">(b688fb1)</span></li>
<li>duchain/parsesession.cpp <span style="color: grey">(f41768a)</span></li>
<li>tests/clang-parser.cpp <span style="color: grey">(3f87d05)</span></li>
<li>tests/test_duchain.cpp <span style="color: grey">(7db9fea)</span></li>
<li>tests/test_problems.cpp <span style="color: grey">(09018d1)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121757/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/01/02/5c0314bf-f241-4ad0-93f6-9a0a48a79135__kdevelop-hasincordef.diff">kdevelop patch</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/01/02/edc11881-06c6-4570-9afd-824a99687116__kdevplatform-hasincordef.diff">platform patch</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>