<table><tr><td style="">thomassc marked 14 inline comments as done.<br />thomassc added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D18224">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D18224#inline-110184">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">parsesession.cpp:505</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">this isn't correct anymore:</p>

<p style="padding: 0; margin: 8px;">When we run into this code path with, say, 5 problems, we would reserve with 5 and then fill the cache. When we then get back here the next time with 0 problems, we would reserve 0, but the size would still be 5!</p>

<p style="padding: 0; margin: 8px;">the easiest solution I can come up with for now is to resize to output_index at the end... but I kind of dislike that we need that to begin with :( can you come up with something better? otherwise, please at least introduce a lambda helper that wraps the "lookup in cache and reuse or insert new problem"</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">To be honest, I am not sure how that cache is supposed to work. I did some testing with debug output added to ParseSession::problemsForFile(), and my impression is that it is currently broken. The behavior seems to be the following:</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">When editing a source file, the cache always starts out empty. problemsForFile() is only called once for that source file. The cache is thus not used.</li>
<li class="remarkup-list-item">When editing a header file, problemsForFile() is called first for that header file and then for some source file which is probably supposed to include that header (but did not always actually do that in my tests). The cache is empty at the start of the first call, so it is not used there. At the start of the second call, it contains the elements added by the header, but the function is now called on the source file, so the cached elements are wrongly used for it. So, the only use of the cache is a wrong use.</li>
</ul>

<p style="padding: 0; margin: 8px;">In case these observations are true and I didn't miss anything, could the cache be simply removed?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D18224#inline-110185">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">parsesession.cpp:541</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">use ki18n too</p>

<p style="padding: 0; margin: 8px;">are we sure that this external problem will always have a location within the current <tt style="background: #ebebeb; font-size: 13px;">file</tt>? couldn't it happen that we encounter completely unrelated errors? I would assume that e.g. in the following scenario, we may end up creating unhelpful issues?</p>

<p style="padding: 0; margin: 8px;">A > B > C</p>

<p style="padding: 0; margin: 8px;">if B has an error due to C, then A shouldn't show anything (except when we "show imports")</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This should only happen in case of missing includes. In this case, it is intentional to show these errors since they may cause other errors later. Other types of problems with location outside of the current file are filtered out above:</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">// missing-include problems are so severe in clang that we always propagate
// them to this document, to ensure that the user will see the error.
if (diagnosticFile != file && ClangDiagnosticEvaluator::diagnosticType(diagnostic) != ClangDiagnosticEvaluator::IncludeFileNotFoundProblem) {
    continue;
}</pre></div></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R32 KDevelop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D18224">https://phabricator.kde.org/D18224</a></div></div><br /><div><strong>To: </strong>thomassc, KDevelop, mwolff<br /><strong>Cc: </strong>aaronpuchert, pino, mwolff, kdevelop-devel, gennad, glebaccon, domson, antismap, iodelay, alexeymin, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd<br /></div>