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

Changeset View

Standalone View

View Options

plugins/clang/duchain/parsesession.cpp

Context not available.
23 23​
24#include "parsesession.h" 24​#include "parsesession.h"
25#include <QStandardPaths> 25​#include <QStandardPaths>
26#include "clangproblem.h" ​
27#include "clangdiagnosticevaluator.h" 26​#include "clangdiagnosticevaluator.h"
28#include "todoextractor.h" 27​#include "todoextractor.h"
29#include "clanghelpers.h" 28​#include "clanghelpers.h"
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…
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.
pinoUnsubmitted
Not Done

ditto

pino: ditto
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?
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…
mwolffUnsubmitted
Done

auto

mwolff: auto
mwolffUnsubmitted
Done

make -> create

mwolff: make -> create
mwolffUnsubmitted
Done

make -> create

mwolff: make -> create
mwolffUnsubmitted
Done

pass a KLocalizedString here

mwolff: pass a `KLocalizedString` here
mwolffUnsubmitted
Done

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

mwolff: once using KLS, use `descriptionTemplate.subs(problem->description()).toString()`
mwolffUnsubmitted
Done

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

mwolff: once using KLS, use `ki18n("Requested here: %1")` here
Context not available.
432 return lang; 431​ return lang;
433} 432​}
434 433​
434​ClangProblem::Ptr ParseSession::getOrCreateProblem(int indexInTU, CXDiagnostic diagnostic) const
435​{
436​ auto& problem = d->m_diagnosticsCache[indexInTU];
437​ if (!problem) {
438​ problem = ClangDiagnosticEvaluator::createProblem(diagnostic, d->m_unit);
439​ }
440​ return problem;
441​}
442​
443​ClangProblem::Ptr ParseSession::createExternalProblem(int indexInTU,
444​ CXDiagnostic diagnostic,
445​ const KLocalizedString& descriptionTemplate,
446​ int childProblemFinalLocationIndex) const
447​{
448​ // Make a copy of the original (cached) problem since it is modified later
449​ auto problem = ClangProblem::Ptr(new ClangProblem(*getOrCreateProblem(indexInTU, diagnostic)));
450​
451​ // Insert a copy of the parent problem (without child problems) as the first
452​ // child problem to preserve its location.
453​ auto* problemCopy = new ClangProblem();
454​ problemCopy->setSource(problem->source());
455​ problemCopy->setFinalLocation(problem->finalLocation());
456​ problemCopy->setFinalLocationMode(problem->finalLocationMode());
457​ problemCopy->setDescription(problem->description());
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
458​ problemCopy->setExplanation(problem->explanation());
459​ problemCopy->setSeverity(problem->severity());
460​
461​ auto childProblems = problem->diagnostics();
462​ childProblems.prepend(IProblem::Ptr(problemCopy));
463​ problem->setDiagnostics(childProblems);
464​
465​ // Override the problem's finalLocation with that of the child problem in this document.
466​ // This is required to make the problem show up in the problem reporter for this
467​ // file, since it filters by finalLocation. It will also lead the user to the correct
468​ // location when clicking the problem and cause proper error highlighting.
469​ int index = (childProblemFinalLocationIndex >= 0) ?
470​ (1 + childProblemFinalLocationIndex) :
471​ (childProblems.size() - 1);
472​ problem->setFinalLocation(childProblems[index]->finalLocation());
473​
474​ problem->setDescription(descriptionTemplate.subs(problem->description()).toString());
475​
476​ return problem;
477​}
478​
479​QList<ClangProblem::Ptr> ParseSession::createRequestedHereProblems(int indexInTU, CXDiagnostic diagnostic, CXFile file) const
480​{
481​ QList<ClangProblem::Ptr> results;
482​
483​ auto childDiagnostics = clang_getChildDiagnostics(diagnostic);
484​ auto numChildDiagnostics = clang_getNumDiagnosticsInSet(childDiagnostics);
485​ for (uint j = 0; j < numChildDiagnostics; ++j) {
486​ auto childDiagnostic = clang_getDiagnosticInSet(childDiagnostics, j);
mwolffUnsubmitted
Done

similarly, create a helper function for this chunk

mwolff: similarly, create a helper function for this chunk
487​ CXSourceLocation childLocation = clang_getDiagnosticLocation(childDiagnostic);
488​ CXFile childDiagnosticFile;
489​ clang_getFileLocation(childLocation, &childDiagnosticFile, nullptr, nullptr, nullptr);
490​ if (childDiagnosticFile == file) {
491​ QString description = ClangString(clang_getDiagnosticSpelling(childDiagnostic)).toString();
492​ if (description.endsWith(QLatin1String("requested here"))) {
493​ // Note: Using the index j here assumes a 1:1 mapping from clang child diagnostics to KDevelop
494​ // problem diagnostics (i.e., child problems). If we wanted to avoid making this assumption, we'd have
495​ // to use ClangDiagnosticEvaluator::createProblem() first and then search within its
496​ // child problems to find the correct index.
497​ results << createExternalProblem(indexInTU, diagnostic, ki18n("Requested here: %1"), j);
498​ }
499​ }
500​ }
501​
502​ return results;
503​}
504​
435QList<ProblemPointer> ParseSession::problemsForFile(CXFile file) const 505​QList<ProblemPointer> ParseSession::problemsForFile(CXFile file) const
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…
436{ 506​{
437 if (!d) { 507​ if (!d) {
Context not available.
451 CXSourceLocation location = clang_getDiagnosticLocation(diagnostic); 521​ CXSourceLocation location = clang_getDiagnosticLocation(diagnostic);
452 CXFile diagnosticFile; 522​ CXFile diagnosticFile;
453 clang_getFileLocation(location, &diagnosticFile, nullptr, nullptr, nullptr); 523​ clang_getFileLocation(location, &diagnosticFile, nullptr, nullptr, nullptr);
524​
525​ auto requestedHereProblems = createRequestedHereProblems(i, diagnostic, file);
526​ for (const auto& ptr : requestedHereProblems) {
527​ problems.append(static_cast<const ProblemPointer&>(ptr));
528​ }
529​
454 // missing-include problems are so severe in clang that we always propagate 530​ // 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. 531​ // them to this document, to ensure that the user will see the error.
456 if (diagnosticFile != file && ClangDiagnosticEvaluator::diagnosticType(diagnostic) != ClangDiagnosticEvaluator::IncludeFileNotFoundProblem) { 532​ if (diagnosticFile != file && ClangDiagnosticEvaluator::diagnosticType(diagnostic) != ClangDiagnosticEvaluator::IncludeFileNotFoundProblem) {
457 continue; 533​ continue;
458 } 534​ }
459 535​
460 auto& problem = d->m_diagnosticsCache[i]; 536​ problems << ((diagnosticFile == file) ?
461 if (!problem) { 537​ getOrCreateProblem(i, diagnostic) :
462 problem = ClangDiagnosticEvaluator::createProblem(diagnostic, d->m_unit); 538​ createExternalProblem(i, diagnostic, ki18n("In included file: %1")));
463 } ​
464 ​
465 problems << problem; ​
466 539​
467 clang_disposeDiagnostic(diagnostic); 540​ clang_disposeDiagnostic(diagnostic);
468 } 541​ }
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…