D18224: Clang Plugin: Report some problems from included files

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added a comment.
This revision now requires changes to proceed.


  nice, this is getting better! some suggestions on how to improve the code quality, and then some potential issues I can think of - please fix or document why they aren't an issue
  
  cheers

INLINE COMMENTS

> parsesession.cpp:205
>  
> +ProblemPointer makeExternalProblem(CXDiagnostic diagnostic, CXTranslationUnit unit,
> +                                   const QString& descriptionTemplate,

make -> create

> parsesession.cpp:206
> +ProblemPointer makeExternalProblem(CXDiagnostic diagnostic, CXTranslationUnit unit,
> +                                   const QString& descriptionTemplate,
> +                                   int childProblemFinalLocationIndex = -1) {

pass a `KLocalizedString` here

> parsesession.cpp:212
> +    // child problem to preserve its location.
> +    ClangProblem* problemCopy = new ClangProblem();
> +    problemCopy->setSource(problem->source());

auto

> parsesession.cpp:233
> +
> +    problem->setDescription(descriptionTemplate.arg(problem->description()));
> +

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

> parsesession.cpp:238
> +
> +QVector<ProblemPointer> makeRequestedHereProblems(CXDiagnostic diagnostic, CXTranslationUnit unit, CXFile file)
> +{

make -> create

> parsesession.cpp:256
> +                // child problems to find the correct index.
> +                results << makeExternalProblem(diagnostic, unit, i18n("Requested here: %1", QLatin1String("%1")), j);
> +            }

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

> parsesession.cpp:505
>      problems.reserve(numDiagnostics);
> -    d->m_diagnosticsCache.resize(numDiagnostics);
> +    d->m_diagnosticsCache.reserve(numDiagnostics);
>  

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"

> parsesession.cpp:541
> +            } else {
> +                problem = makeExternalProblem(diagnostic, d->m_unit, i18n("In included file: %1", QLatin1String("%1")));
> +            }

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")

> test_duchain.cpp:1947
> +    QString sourceCode = "#include \"" + header.url().str() + "\"\n";
> +    sourceCode += QStringLiteral(R"(
> +struct A {};

QStringLiteral(R"(
#include "%1"
...
").arg(header.url().str());

> test_duchain.cpp:1965
> +
> +    TopDUContext* headerCtx = dynamic_cast<TopDUContext*>(top->importedParentContexts().first().context(top));
> +    QVERIFY(headerCtx);

auto

> test_duchain.cpp:2019
> +
> +    QString sourceCode = "#include \"" + header.url().str() + "\"\n";
> +    sourceCode += QStringLiteral(R"(

see above

> test_duchain.cpp:2044
> +
> +    TopDUContext* headerCtx = dynamic_cast<TopDUContext*>(top->importedParentContexts().first().context(top));
> +    QVERIFY(headerCtx);

auto

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D18224

To: thomassc, #kdevelop, mwolff
Cc: aaronpuchert, pino, mwolff, kdevelop-devel, gennad, glebaccon, domson, antismap, iodelay, alexeymin, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20190310/ea35589e/attachment-0001.html>