Differential D18224 Diff 55563 plugins/clang/duchain/parsesession.cpp

Changeset View

Standalone View

View Options

plugins/clang/duchain/parsesession.cpp

Context not available.
202 }) != includePaths.end(); 202​ }) != includePaths.end();
203} 203​}
204 204​
205​ProblemPointer createExternalProblem(CXDiagnostic diagnostic, CXTranslationUnit unit,
mwolffUnsubmitted
Done

make -> create

mwolff: make -> create
206​ const KLocalizedString& descriptionTemplate,
mwolffUnsubmitted
Done

pass a KLocalizedString here

mwolff: pass a `KLocalizedString` here
207​ int childProblemFinalLocationIndex = -1) {
208​ auto problem = ClangDiagnosticEvaluator::createProblem(diagnostic, unit);
209​
210​ // Insert a copy of the parent problem (without child problems) as the first
211​ // child problem to preserve its location.
212​ auto* problemCopy = new ClangProblem();
mwolffUnsubmitted
Done

auto

mwolff: auto
213​ problemCopy->setSource(problem->source());
pinoUnsubmitted
Not Done

OOC, does clang support translations of its messages? if so, would this check be problematic?

pino: OOC, does clang support translations of its messages? if so, would this check be problematic?
aaronpuchertUnsubmitted
Not Done

Not yet, but it seems they want to.

aaronpuchert: [Not yet](http://clang.llvm.org/docs/InternalsManual.html#internals-diag-translation), but it…
214​ problemCopy->setFinalLocation(problem->finalLocation());
215​ problemCopy->setFinalLocationMode(problem->finalLocationMode());
216​ problemCopy->setDescription(problem->description());
217​ problemCopy->setExplanation(problem->explanation());
218​ problemCopy->setSeverity(problem->severity());
219​
220​ auto childProblems = problem->diagnostics();
221​ childProblems.prepend(IProblem::Ptr(problemCopy));
222​ problem->setDiagnostics(childProblems);
223​
224​ // Override the problem's finalLocation with that of the child problem in this document.
225​ // This is required to make the problem show up in the problem reporter for this
226​ // file, since it filters by finalLocation. It will also lead the user to the correct
227​ // location when clicking the problem and cause proper error highlighting.
228​ int index = (childProblemFinalLocationIndex >= 0) ?
229​ (1 + childProblemFinalLocationIndex) :
230​ (childProblems.size() - 1);
231​ problem->setFinalLocation(childProblems[index]->finalLocation());
232​
233​ problem->setDescription(descriptionTemplate.subs(problem->description()).toString());
mwolffUnsubmitted
Done

once using KLS, use descriptionTemplate.subs(problem->description()).toString()

mwolff: once using KLS, use `descriptionTemplate.subs(problem->description()).toString()`
234​
235​ return ProblemPointer(problem);
236​}
237​
238​QList<ProblemPointer> createRequestedHereProblems(CXDiagnostic diagnostic, CXTranslationUnit unit, CXFile file)
mwolffUnsubmitted
Done

make -> create

mwolff: make -> create
239​{
240​ QList<ProblemPointer> results;
aaronpuchertUnsubmitted
Not Done

Wouldn't it be easier to relax that filtering?

aaronpuchert: Wouldn't it be easier to relax that filtering?
mwolffUnsubmitted
Not Done

that's not so easy either, since it would require to recursively iterate through all imported contexts in the current file and then find ones that are associated with the current one. I.e. that would basically mean we always "show imported", which can become slow. So I would prefer to do it here, as suggested by this patch

mwolff: that's not so easy either, since it would require to recursively iterate through all imported…
241​
242​ auto childDiagnostics = clang_getChildDiagnostics(diagnostic);
243​ auto numChildDiagnostics = clang_getNumDiagnosticsInSet(childDiagnostics);
244​ for (uint j = 0; j < numChildDiagnostics; ++j) {
245​ auto childDiagnostic = clang_getDiagnosticInSet(childDiagnostics, j);
246​ CXSourceLocation childLocation = clang_getDiagnosticLocation(childDiagnostic);
247​ CXFile childDiagnosticFile;
248​ clang_getFileLocation(childLocation, &childDiagnosticFile, nullptr, nullptr, nullptr);
249​ if (childDiagnosticFile == file) {
250​ QString description = ClangString(clang_getDiagnosticSpelling(childDiagnostic)).toString();
251​ if (description.endsWith(QLatin1String("requested here"))) {
pinoUnsubmitted
Not Done

no string puzzles please:
https://techbase.kde.org/Development/Tutorials/Localization/i18n_Mistakes#Pitfall_.232:_Word_Puzzles
so i18n("Requested here: %1", problem->description())

pino: no string puzzles please: https://techbase.kde.
mwolffUnsubmitted
Not Done

can't we use the original description of the child diagnostic?

mwolff: can't we use the original description of the child diagnostic?
252​ // Note: Using the index j here assumes a 1:1 mapping from clang child diagnostics to KDevelop
253​ // problem diagnostics (i.e., child problems). If we wanted to avoid making this assumption, we'd have
254​ // to use ClangDiagnosticEvaluator::createProblem() first and then search within its
255​ // child problems to find the correct index.
256​ results << createExternalProblem(diagnostic, unit, ki18n("Requested here: %1"), j);
mwolffUnsubmitted
Done

once using KLS, use ki18n("Requested here: %1") here

mwolff: once using KLS, use `ki18n("Requested here: %1")` here
257​ }
258​ }
259​ }
260​
261​ return results;
262​}
pinoUnsubmitted
Not Done

ditto

pino: ditto
263​
205} 264​}
206 265​
207ParseSessionData::ParseSessionData(const QVector<UnsavedFile>& unsavedFiles, ClangIndex* index, 266​ParseSessionData::ParseSessionData(const QVector<UnsavedFile>& unsavedFiles, ClangIndex* index,
Context not available.
373void ParseSessionData::setUnit(CXTranslationUnit unit) 432​void ParseSessionData::setUnit(CXTranslationUnit unit)
374{ 433​{
375 m_unit = unit; 434​ m_unit = unit;
376 m_diagnosticsCache.clear(); ​
377 if (m_unit) { 435​ if (m_unit) {
378 const ClangString unitFile(clang_getTranslationUnitSpelling(unit)); 436​ const ClangString unitFile(clang_getTranslationUnitSpelling(unit));
379 m_file = clang_getFile(m_unit, unitFile.c_str()); 437​ m_file = clang_getFile(m_unit, unitFile.c_str());
mwolffUnsubmitted
Done

put this into a helper function to reduce the complexity here

mwolff: put this into a helper function to reduce the complexity here
mwolffUnsubmitted
Done

similarly, create a helper function for this chunk

mwolff: similarly, create a helper function for this chunk
Context not available.
443 // extra clang diagnostics 501​ // extra clang diagnostics
444 const uint numDiagnostics = clang_getNumDiagnostics(d->m_unit); 502​ const uint numDiagnostics = clang_getNumDiagnostics(d->m_unit);
445 problems.reserve(numDiagnostics); 503​ problems.reserve(numDiagnostics);
446 d->m_diagnosticsCache.resize(numDiagnostics); ​
447 504​
448 for (uint i = 0; i < numDiagnostics; ++i) { 505​ for (uint i = 0; i < numDiagnostics; ++i) {
mwolffUnsubmitted
Not Done

this isn't correct anymore:

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!

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"

mwolff: this isn't correct anymore: When we run into this code path with, say, 5 problems, we would…
thomasscAuthorUnsubmitted
Not Done

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:

  • 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.
  • 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.

In case these observations are true and I didn't miss anything, could the cache be simply removed?

thomassc: To be honest, I am not sure how that cache is supposed to work. I did some testing with debug…
449 auto diagnostic = clang_getDiagnostic(d->m_unit, i); 506​ auto diagnostic = clang_getDiagnostic(d->m_unit, i);
Context not available.
451 CXSourceLocation location = clang_getDiagnosticLocation(diagnostic); 508​ CXSourceLocation location = clang_getDiagnosticLocation(diagnostic);
452 CXFile diagnosticFile; 509​ CXFile diagnosticFile;
453 clang_getFileLocation(location, &diagnosticFile, nullptr, nullptr, nullptr); 510​ clang_getFileLocation(location, &diagnosticFile, nullptr, nullptr, nullptr);
511​
512​ problems.append(createRequestedHereProblems(diagnostic, d->m_unit, file));
513​
454 // missing-include problems are so severe in clang that we always propagate 514​ // missing-include problems are so severe in clang that we always propagate
455 // them to this document, to ensure that the user will see the error. 515​ // them to this document, to ensure that the user will see the error.
456 if (diagnosticFile != file && ClangDiagnosticEvaluator::diagnosticType(diagnostic) != ClangDiagnosticEvaluator::IncludeFileNotFoundProblem) { 516​ if (diagnosticFile != file && ClangDiagnosticEvaluator::diagnosticType(diagnostic) != ClangDiagnosticEvaluator::IncludeFileNotFoundProblem) {
457 continue; 517​ continue;
458 } 518​ }
459 519​
460 auto& problem = d->m_diagnosticsCache[i]; 520​ problems << ((diagnosticFile == file) ?
461 if (!problem) { 521​ ProblemPointer(ClangDiagnosticEvaluator::createProblem(diagnostic, d->m_unit)) :
462 problem = ClangDiagnosticEvaluator::createProblem(diagnostic, d->m_unit); 522​ createExternalProblem(diagnostic, d->m_unit, ki18n("In included file: %1")));
463 } ​
464 ​
465 problems << problem; ​
466 523​
467 clang_disposeDiagnostic(diagnostic); 524​ clang_disposeDiagnostic(diagnostic);
468 } 525​ }
Context not available.
mwolffUnsubmitted
Not Done

use ki18n too

are we sure that this external problem will always have a location within the current file? 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?

A > B > C

if B has an error due to C, then A shouldn't show anything (except when we "show imports")

mwolff: use ki18n too are we sure that this external problem will always have a location within the…
thomasscAuthorUnsubmitted
Not Done

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:

// 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;
}
thomassc: This should only happen in case of missing includes. In this case, it is intentional to show…