From 5761caff9e94c657f14f1dc0b3566f1d3dba16b5 Mon Sep 17 00:00:00 2001 From: Ivan Donchevskii Date: Fri, 7 Dec 2018 16:36:27 +0100 Subject: [PATCH] ClangTools: Improve fix-its handling Two areas are touched: 1. Select multiple diagnostics if they have the same set of fix-its. 2. Watch the files and invalidate diagnostics if corresponding file was edited. Change-Id: If4487ba91f45c25d1aed1a98990dd9b6df9d7fe2 Reviewed-by: Marco Bubke --- src/plugins/clangtools/clangtidyclazytool.cpp | 15 +++- src/plugins/clangtools/clangtoolsdiagnostic.h | 10 ++- .../clangtools/clangtoolsdiagnosticmodel.cpp | 68 ++++++++++++++++++- .../clangtools/clangtoolsdiagnosticmodel.h | 25 ++++++- .../debugger/analyzer/diagnosticlocation.cpp | 6 ++ .../debugger/analyzer/diagnosticlocation.h | 1 + 6 files changed, 116 insertions(+), 9 deletions(-) diff --git a/src/plugins/clangtools/clangtidyclazytool.cpp b/src/plugins/clangtools/clangtidyclazytool.cpp index a58cb6f1d4a..bc169a0e9be 100644 --- a/src/plugins/clangtools/clangtidyclazytool.cpp +++ b/src/plugins/clangtools/clangtidyclazytool.cpp @@ -102,6 +102,9 @@ public: static void addFixitOperations(DiagnosticItem *diagnosticItem, const FixitsRefactoringFile &file, bool apply) { + if (!diagnosticItem->hasNewFixIts()) + return; + // Did we already created the fixit operations? ReplacementOperations currentOps = diagnosticItem->fixitOperations(); if (!currentOps.isEmpty()) { @@ -135,7 +138,7 @@ public: diagnosticItem->setFixitOperations(replacements); } - void apply() + void apply(ClangToolsDiagnosticModel *model) { for (auto it = m_refactoringFileInfos.begin(); it != m_refactoringFileInfos.end(); ++it) { RefactoringFileInfo &fileInfo = it.value(); @@ -167,18 +170,24 @@ public: for (DiagnosticItem *item : itemsScheduledOrSchedulable) ops += item->fixitOperations(); + if (ops.empty()) + continue; + // Apply file QVector itemsApplied; QVector itemsFailedToApply; QVector itemsInvalidated; fileInfo.file.setReplacements(ops); + + model->removeWatchedPath(ops.first()->fileName); if (fileInfo.file.apply()) { itemsApplied = itemsScheduled; } else { itemsFailedToApply = itemsScheduled; itemsInvalidated = itemsSchedulable; } + model->addWatchedPath(ops.first()->fileName); // Update DiagnosticItem state for (DiagnosticItem *diagnosticItem : itemsScheduled) @@ -262,7 +271,7 @@ ClangTidyClazyTool::ClangTidyClazyTool() diagnosticItems += static_cast(item); }); - ApplyFixIts(diagnosticItems).apply(); + ApplyFixIts(diagnosticItems).apply(m_diagnosticModel); }); ActionContainer *menu = ActionManager::actionContainer(Debugger::Constants::M_DEBUG_ANALYZER); @@ -354,7 +363,9 @@ void ClangTidyClazyTool::startTool(bool askUserForFileSelection) m_perspective.select(); + m_diagnosticModel->clearAndSetupCache(); m_diagnosticModel->clear(); + setToolBusy(true); m_diagnosticFilterModel->setProject(project); m_running = true; diff --git a/src/plugins/clangtools/clangtoolsdiagnostic.h b/src/plugins/clangtools/clangtoolsdiagnostic.h index 98ebb0173d6..9f6b5a99690 100644 --- a/src/plugins/clangtools/clangtoolsdiagnostic.h +++ b/src/plugins/clangtools/clangtoolsdiagnostic.h @@ -27,9 +27,9 @@ #include -#include #include #include +#include namespace ClangTools { namespace Internal { @@ -38,11 +38,15 @@ class ExplainingStep { public: bool isValid() const; + bool operator<(const ExplainingStep &other) const { + return std::tie(location, ranges, message) + < std::tie(other.location, other.ranges, other.message); + } QString message; QString extendedMessage; Debugger::DiagnosticLocation location; - QList ranges; + QVector ranges; int depth = 0; bool isFixIt = false; }; @@ -58,7 +62,7 @@ public: QString issueContextKind; QString issueContext; Debugger::DiagnosticLocation location; - QList explainingSteps; + QVector explainingSteps; bool hasFixits = false; }; diff --git a/src/plugins/clangtools/clangtoolsdiagnosticmodel.cpp b/src/plugins/clangtools/clangtoolsdiagnosticmodel.cpp index 977b264afcc..50b666b7af6 100644 --- a/src/plugins/clangtools/clangtoolsdiagnosticmodel.cpp +++ b/src/plugins/clangtools/clangtoolsdiagnosticmodel.cpp @@ -54,8 +54,10 @@ private: ClangToolsDiagnosticModel::ClangToolsDiagnosticModel(QObject *parent) : Utils::TreeModel<>(parent) + , m_filesWatcher(std::make_unique()) { setHeader({tr("Issue"), tr("Location"), tr("Fixit Status")}); + connectFileWatcher(); } void ClangToolsDiagnosticModel::addDiagnostics(const QList &diagnostics) @@ -68,8 +70,11 @@ void ClangToolsDiagnosticModel::addDiagnostics(const QList &diagnost emit fixItsToApplyCountChanged(m_fixItsToApplyCount); }; + if (!diagnostics.empty()) + addWatchedPath(diagnostics.front().location.filePath); + for (const Diagnostic &d : diagnostics) - rootItem()->appendChild(new DiagnosticItem(d, onFixitStatusChanged)); + rootItem()->appendChild(new DiagnosticItem(d, onFixitStatusChanged, this)); } QList ClangToolsDiagnosticModel::diagnostics() const @@ -85,6 +90,49 @@ int ClangToolsDiagnosticModel::diagnosticsCount() const return rootItem()->childCount(); } +void ClangToolsDiagnosticModel::updateItems(const DiagnosticItem *changedItem) +{ + for (auto item : stepsToItemsCache[changedItem->diagnostic().explainingSteps]) { + if (item != changedItem) + item->setFixItStatus(changedItem->fixItStatus()); + } +} + +void ClangToolsDiagnosticModel::connectFileWatcher() +{ + connect(m_filesWatcher.get(), + &QFileSystemWatcher::fileChanged, + this, + &ClangToolsDiagnosticModel::onFileChanged); +} + +void ClangToolsDiagnosticModel::clearAndSetupCache() +{ + m_filesWatcher = std::make_unique(); + connectFileWatcher(); + stepsToItemsCache.clear(); +} + +void ClangToolsDiagnosticModel::onFileChanged(const QString &path) +{ + for (Utils::TreeItem * const item : *rootItem()) { + auto diagnosticItem = static_cast(item); + if (diagnosticItem->diagnostic().location.filePath == path) + diagnosticItem->setFixItStatus(FixitStatus::Invalidated); + } + removeWatchedPath(path); +} + +void ClangToolsDiagnosticModel::removeWatchedPath(const QString &path) +{ + m_filesWatcher->removePath(path); +} + +void ClangToolsDiagnosticModel::addWatchedPath(const QString &path) +{ + m_filesWatcher->addPath(path); +} + static QString createDiagnosticToolTipString(const Diagnostic &diagnostic) { using StringPair = QPair; @@ -216,10 +264,12 @@ static QString fullText(const Diagnostic &diagnostic) return text; } - -DiagnosticItem::DiagnosticItem(const Diagnostic &diag, const OnFixitStatusChanged &onFixitStatusChanged) +DiagnosticItem::DiagnosticItem(const Diagnostic &diag, + const OnFixitStatusChanged &onFixitStatusChanged, + ClangToolsDiagnosticModel *parent) : m_diagnostic(diag) , m_onFixitStatusChanged(onFixitStatusChanged) + , m_parentModel(parent) { if (diag.hasFixits) m_fixitStatus = FixitStatus::NotScheduled; @@ -231,6 +281,9 @@ DiagnosticItem::DiagnosticItem(const Diagnostic &diag, const OnFixitStatusChange return; } + if (!diag.explainingSteps.isEmpty()) + m_parentModel->stepsToItemsCache[diag.explainingSteps].push_back(this); + foreach (const ExplainingStep &s, diag.explainingSteps) appendChild(new ExplainingStepItem(s)); } @@ -336,6 +389,7 @@ bool DiagnosticItem::setData(int column, const QVariant &data, int role) : FixitStatus::NotScheduled; setFixItStatus(newStatus); + m_parentModel->updateItems(this); return true; } @@ -357,6 +411,14 @@ void DiagnosticItem::setFixitOperations(const ReplacementOperations &replacement m_fixitOperations = replacements; } +bool DiagnosticItem::hasNewFixIts() const +{ + if (m_diagnostic.explainingSteps.empty()) + return false; + + return m_parentModel->stepsToItemsCache[m_diagnostic.explainingSteps].front() == this; +} + ExplainingStepItem::ExplainingStepItem(const ExplainingStep &step) : m_step(step) { } diff --git a/src/plugins/clangtools/clangtoolsdiagnosticmodel.h b/src/plugins/clangtools/clangtoolsdiagnosticmodel.h index 998989699eb..b4c108f6f78 100644 --- a/src/plugins/clangtools/clangtoolsdiagnosticmodel.h +++ b/src/plugins/clangtools/clangtoolsdiagnosticmodel.h @@ -33,10 +33,14 @@ #include #include +#include #include #include +#include #include +#include +#include namespace ProjectExplorer { class Project; } @@ -52,11 +56,14 @@ enum class FixitStatus { Invalidated, }; +class ClangToolsDiagnosticModel; + class DiagnosticItem : public Utils::TreeItem { public: using OnFixitStatusChanged = std::function; - DiagnosticItem(const Diagnostic &diag, const OnFixitStatusChanged &onFixitStatusChanged); + DiagnosticItem(const Diagnostic &diag, const OnFixitStatusChanged &onFixitStatusChanged, + ClangToolsDiagnosticModel *parent); ~DiagnosticItem() override; const Diagnostic &diagnostic() const { return m_diagnostic; } @@ -64,6 +71,7 @@ public: FixitStatus fixItStatus() const { return m_fixitStatus; } void setFixItStatus(const FixitStatus &status); + bool hasNewFixIts() const; ReplacementOperations &fixitOperations() { return m_fixitOperations; } void setFixitOperations(const ReplacementOperations &replacements); @@ -78,12 +86,15 @@ private: ReplacementOperations m_fixitOperations; FixitStatus m_fixitStatus = FixitStatus::NotAvailable; + ClangToolsDiagnosticModel *m_parentModel = nullptr; }; class ClangToolsDiagnosticModel : public Utils::TreeModel<> { Q_OBJECT + friend class DiagnosticItem; + public: ClangToolsDiagnosticModel(QObject *parent = nullptr); @@ -96,10 +107,22 @@ public: DiagnosticRole = Debugger::DetailedErrorView::FullTextRole + 1 }; + void clearAndSetupCache(); + void removeWatchedPath(const QString &path); + void addWatchedPath(const QString &path); + signals: void fixItsToApplyCountChanged(int count); private: + void connectFileWatcher(); + void updateItems(const DiagnosticItem *changedItem); + void onFileChanged(const QString &path); + +private: + std::map, QVector> stepsToItemsCache; + std::unique_ptr m_filesWatcher; + QVector m_allowFileWriteOnce; int m_fixItsToApplyCount = 0; }; diff --git a/src/plugins/debugger/analyzer/diagnosticlocation.cpp b/src/plugins/debugger/analyzer/diagnosticlocation.cpp index 095101d6ca1..417edcab53f 100644 --- a/src/plugins/debugger/analyzer/diagnosticlocation.cpp +++ b/src/plugins/debugger/analyzer/diagnosticlocation.cpp @@ -46,6 +46,12 @@ bool operator==(const DiagnosticLocation &first, const DiagnosticLocation &secon && first.column == second.column; } +bool operator<(const DiagnosticLocation &first, const DiagnosticLocation &second) +{ + return std::tie(first.filePath, first.line, first.column) + < std::tie(second.filePath, second.line, second.column); +} + QDebug operator<<(QDebug dbg, const DiagnosticLocation &location) { dbg.nospace() << "Location(" << location.filePath << ", " diff --git a/src/plugins/debugger/analyzer/diagnosticlocation.h b/src/plugins/debugger/analyzer/diagnosticlocation.h index 5bf95bff74d..69b4ef091a1 100644 --- a/src/plugins/debugger/analyzer/diagnosticlocation.h +++ b/src/plugins/debugger/analyzer/diagnosticlocation.h @@ -49,6 +49,7 @@ public: }; DEBUGGER_EXPORT bool operator==(const DiagnosticLocation &first, const DiagnosticLocation &second); +DEBUGGER_EXPORT bool operator<(const DiagnosticLocation &first, const DiagnosticLocation &second); DEBUGGER_EXPORT QDebug operator<<(QDebug dbg, const DiagnosticLocation &location); } // namespace Debugger