From f7741ef63ce965512fa672fdbbba964144a2f20a Mon Sep 17 00:00:00 2001 From: Nikolai Kosjar Date: Fri, 18 Sep 2015 17:01:32 +0200 Subject: [PATCH] Clang: Report only diagnostics that can be shown in the editor Change-Id: I9c258159d240c6ba7eeff34702d8512d9220b3af Reviewed-by: Marco Bubke --- .../sourcelocationcontainer.cpp | 8 +- .../clangbackendipc/sourcelocationcontainer.h | 2 + .../clangbackend/ipcsource/clangipcserver.cpp | 2 +- .../clangbackend/ipcsource/diagnostic.cpp | 11 ++- src/tools/clangbackend/ipcsource/diagnostic.h | 4 + .../clangbackend/ipcsource/diagnosticset.cpp | 14 ++- .../clangbackend/ipcsource/diagnosticset.h | 4 + .../ipcsource/translationunit.cpp | 12 +++ .../clangbackend/ipcsource/translationunit.h | 2 + .../ipcsource/translationunits.cpp | 2 +- .../data/diagnostic_diagnosticset_header.cpp | 2 + .../diagnostic_diagnosticset_mainfile.cpp | 4 + tests/unit/unittest/diagnosticsettest.cpp | 88 ++++++++++++++++++- tests/unit/unittest/diagnostictest.cpp | 67 +++++++++++++- .../unittest/matcher-diagnosticcontainer.h | 63 +++++++++++++ tests/unit/unittest/unittest.pro | 3 +- 16 files changed, 278 insertions(+), 10 deletions(-) create mode 100644 tests/unit/unittest/data/diagnostic_diagnosticset_header.cpp create mode 100644 tests/unit/unittest/data/diagnostic_diagnosticset_mainfile.cpp create mode 100644 tests/unit/unittest/matcher-diagnosticcontainer.h diff --git a/src/libs/clangbackendipc/sourcelocationcontainer.cpp b/src/libs/clangbackendipc/sourcelocationcontainer.cpp index 634e01e87d..82bf23237f 100644 --- a/src/libs/clangbackendipc/sourcelocationcontainer.cpp +++ b/src/libs/clangbackendipc/sourcelocationcontainer.cpp @@ -82,7 +82,13 @@ QDataStream &operator>>(QDataStream &in, SourceLocationContainer &container) bool operator==(const SourceLocationContainer &first, const SourceLocationContainer &second) { - return first.offset_ == second.offset_ && first.filePath_ == second.filePath_; + return !(first != second); +} + +bool operator!=(const SourceLocationContainer &first, const SourceLocationContainer &second) +{ + return first.offset_ != second.offset_ + || first.filePath_ != second.filePath_; } bool operator<(const SourceLocationContainer &first, const SourceLocationContainer &second) diff --git a/src/libs/clangbackendipc/sourcelocationcontainer.h b/src/libs/clangbackendipc/sourcelocationcontainer.h index bfe446ec05..54e40f479b 100644 --- a/src/libs/clangbackendipc/sourcelocationcontainer.h +++ b/src/libs/clangbackendipc/sourcelocationcontainer.h @@ -44,6 +44,7 @@ class CMBIPC_EXPORT SourceLocationContainer friend CMBIPC_EXPORT QDataStream &operator<<(QDataStream &out, const SourceLocationContainer &container); friend CMBIPC_EXPORT QDataStream &operator>>(QDataStream &in, SourceLocationContainer &container); friend CMBIPC_EXPORT bool operator==(const SourceLocationContainer &first, const SourceLocationContainer &second); + friend CMBIPC_EXPORT bool operator!=(const SourceLocationContainer &first, const SourceLocationContainer &second); friend CMBIPC_EXPORT bool operator<(const SourceLocationContainer &first, const SourceLocationContainer &second); public: @@ -65,6 +66,7 @@ private: CMBIPC_EXPORT QDataStream &operator<<(QDataStream &out, const SourceLocationContainer &container); CMBIPC_EXPORT QDataStream &operator>>(QDataStream &in, SourceLocationContainer &container); CMBIPC_EXPORT bool operator==(const SourceLocationContainer &first, const SourceLocationContainer &second); +CMBIPC_EXPORT bool operator!=(const SourceLocationContainer &first, const SourceLocationContainer &second); CMBIPC_EXPORT bool operator<(const SourceLocationContainer &first, const SourceLocationContainer &second); CMBIPC_EXPORT QDebug operator<<(QDebug debug, const SourceLocationContainer &container); diff --git a/src/tools/clangbackend/ipcsource/clangipcserver.cpp b/src/tools/clangbackend/ipcsource/clangipcserver.cpp index f6d10d2f9c..320c982c88 100644 --- a/src/tools/clangbackend/ipcsource/clangipcserver.cpp +++ b/src/tools/clangbackend/ipcsource/clangipcserver.cpp @@ -233,7 +233,7 @@ void ClangIpcServer::requestDiagnostics(const RequestDiagnosticsMessage &message message.file().projectPartId()); client()->diagnosticsChanged(DiagnosticsChangedMessage(translationUnit.fileContainer(), - translationUnit.diagnostics().toDiagnosticContainers())); + translationUnit.mainFileDiagnostics())); } catch (const TranslationUnitDoesNotExistException &exception) { client()->translationUnitDoesNotExist(TranslationUnitDoesNotExistMessage(exception.fileContainer())); } catch (const ProjectPartDoNotExistException &exception) { diff --git a/src/tools/clangbackend/ipcsource/diagnostic.cpp b/src/tools/clangbackend/ipcsource/diagnostic.cpp index e851a7c730..fdb7a0940a 100644 --- a/src/tools/clangbackend/ipcsource/diagnostic.cpp +++ b/src/tools/clangbackend/ipcsource/diagnostic.cpp @@ -138,7 +138,7 @@ DiagnosticSet Diagnostic::childDiagnostics() const return DiagnosticSet(clang_getChildDiagnostics(cxDiagnostic)); } -DiagnosticContainer Diagnostic::toDiagnosticContainer() const +DiagnosticContainer Diagnostic::toDiagnosticContainer(const IsAcceptedDiagnostic &isAcceptedChildDiagnostic) const { return DiagnosticContainer(text(), category(), @@ -147,7 +147,14 @@ DiagnosticContainer Diagnostic::toDiagnosticContainer() const location().toSourceLocationContainer(), getSourceRangeContainers(), getFixItContainers(), - childDiagnostics().toDiagnosticContainers()); + childDiagnostics().toDiagnosticContainers(isAcceptedChildDiagnostic)); +} + +DiagnosticContainer Diagnostic::toDiagnosticContainer() const +{ + const auto acceptAllDiagnostics = [](const Diagnostic &) { return true; }; + + return toDiagnosticContainer(acceptAllDiagnostics); } QVector Diagnostic::getSourceRangeContainers() const diff --git a/src/tools/clangbackend/ipcsource/diagnostic.h b/src/tools/clangbackend/ipcsource/diagnostic.h index 5db8ad45bf..279e97fd9e 100644 --- a/src/tools/clangbackend/ipcsource/diagnostic.h +++ b/src/tools/clangbackend/ipcsource/diagnostic.h @@ -35,6 +35,7 @@ #include +#include #include class Utf8String; @@ -75,6 +76,9 @@ public: std::vector fixIts() const; DiagnosticSet childDiagnostics() const; + using IsAcceptedDiagnostic = std::function; + DiagnosticContainer toDiagnosticContainer( + const IsAcceptedDiagnostic &isAcceptedChildDiagnostic) const; DiagnosticContainer toDiagnosticContainer() const; private: diff --git a/src/tools/clangbackend/ipcsource/diagnosticset.cpp b/src/tools/clangbackend/ipcsource/diagnosticset.cpp index 44f4071a8c..1207bd71fd 100644 --- a/src/tools/clangbackend/ipcsource/diagnosticset.cpp +++ b/src/tools/clangbackend/ipcsource/diagnosticset.cpp @@ -86,12 +86,22 @@ DiagnosticSet::ConstIterator DiagnosticSet::end() const } QVector DiagnosticSet::toDiagnosticContainers() const +{ + const auto isAcceptedDiagnostic = [](const Diagnostic &) { return true; }; + + return toDiagnosticContainers(isAcceptedDiagnostic); +} + +QVector DiagnosticSet::toDiagnosticContainers( + const Diagnostic::IsAcceptedDiagnostic &isAcceptedDiagnostic) const { QVector diagnosticContainers; diagnosticContainers.reserve(size()); - for (const Diagnostic &diagnostic : *this) - diagnosticContainers.push_back(diagnostic.toDiagnosticContainer()); + for (const Diagnostic &diagnostic : *this) { + if (isAcceptedDiagnostic(diagnostic)) + diagnosticContainers.push_back(diagnostic.toDiagnosticContainer(isAcceptedDiagnostic)); + } return diagnosticContainers; } diff --git a/src/tools/clangbackend/ipcsource/diagnosticset.h b/src/tools/clangbackend/ipcsource/diagnosticset.h index bc9b8431e6..ce65fcdd29 100644 --- a/src/tools/clangbackend/ipcsource/diagnosticset.h +++ b/src/tools/clangbackend/ipcsource/diagnosticset.h @@ -38,6 +38,8 @@ #include +#include + namespace ClangBackEnd { class DiagnosticSetIterator; @@ -71,6 +73,8 @@ public: ConstIterator end() const; QVector toDiagnosticContainers() const; + QVector toDiagnosticContainers( + const Diagnostic::IsAcceptedDiagnostic &isAcceptedDiagnostic) const; private: DiagnosticSet(CXDiagnosticSet cxDiagnosticSet); diff --git a/src/tools/clangbackend/ipcsource/translationunit.cpp b/src/tools/clangbackend/ipcsource/translationunit.cpp index 1f483244bf..88e917010f 100644 --- a/src/tools/clangbackend/ipcsource/translationunit.cpp +++ b/src/tools/clangbackend/ipcsource/translationunit.cpp @@ -33,8 +33,10 @@ #include "clangstring.h" #include "codecompleter.h" #include "commandlinearguments.h" +#include "diagnosticcontainer.h" #include "diagnosticset.h" #include "projectpart.h" +#include "sourcelocation.h" #include "translationunitfilenotexitexception.h" #include "translationunitisnullexception.h" #include "translationunitparseerrorexception.h" @@ -218,6 +220,16 @@ DiagnosticSet TranslationUnit::diagnostics() const return DiagnosticSet(clang_getDiagnosticSetFromTU(cxTranslationUnit())); } +QVector TranslationUnit::mainFileDiagnostics() const +{ + const auto mainFilePath = filePath(); + const auto isMainFileDiagnostic = [mainFilePath](const Diagnostic &diagnostic) { + return diagnostic.location().filePath() == mainFilePath; + }; + + return diagnostics().toDiagnosticContainers(isMainFileDiagnostic); +} + const QSet &TranslationUnit::dependedFilePaths() const { createTranslationUnitIfNeeded(); diff --git a/src/tools/clangbackend/ipcsource/translationunit.h b/src/tools/clangbackend/ipcsource/translationunit.h index 0b22359d74..e44852e473 100644 --- a/src/tools/clangbackend/ipcsource/translationunit.h +++ b/src/tools/clangbackend/ipcsource/translationunit.h @@ -49,6 +49,7 @@ class TranslationUnitData; class CodeCompleter; class UnsavedFiles; class ProjectPart; +class DiagnosticContainer; class DiagnosticSet; class FileContainer; class TranslationUnits; @@ -102,6 +103,7 @@ public: bool hasNewDiagnostics() const; DiagnosticSet diagnostics() const; + QVector mainFileDiagnostics() const; const QSet &dependedFilePaths() const; diff --git a/src/tools/clangbackend/ipcsource/translationunits.cpp b/src/tools/clangbackend/ipcsource/translationunits.cpp index c542b2d98c..8fd3b14b7c 100644 --- a/src/tools/clangbackend/ipcsource/translationunits.cpp +++ b/src/tools/clangbackend/ipcsource/translationunits.cpp @@ -291,7 +291,7 @@ void TranslationUnits::sendDiagnosticChangedMessage(const TranslationUnit &trans { if (sendDiagnosticsChangedCallback) { DiagnosticsChangedMessage message(translationUnit.fileContainer(), - translationUnit.diagnostics().toDiagnosticContainers()); + translationUnit.mainFileDiagnostics()); sendDiagnosticsChangedCallback(std::move(message)); } diff --git a/tests/unit/unittest/data/diagnostic_diagnosticset_header.cpp b/tests/unit/unittest/data/diagnostic_diagnosticset_header.cpp new file mode 100644 index 0000000000..cf05802cb6 --- /dev/null +++ b/tests/unit/unittest/data/diagnostic_diagnosticset_header.cpp @@ -0,0 +1,2 @@ +void f() {} + diff --git a/tests/unit/unittest/data/diagnostic_diagnosticset_mainfile.cpp b/tests/unit/unittest/data/diagnostic_diagnosticset_mainfile.cpp new file mode 100644 index 0000000000..323842ca58 --- /dev/null +++ b/tests/unit/unittest/data/diagnostic_diagnosticset_mainfile.cpp @@ -0,0 +1,4 @@ +#include "diagnostic_diagnosticset_header.cpp" + +void f() {} + diff --git a/tests/unit/unittest/diagnosticsettest.cpp b/tests/unit/unittest/diagnosticsettest.cpp index 6b5da58a10..a3d89e5105 100644 --- a/tests/unit/unittest/diagnosticsettest.cpp +++ b/tests/unit/unittest/diagnosticsettest.cpp @@ -28,9 +28,15 @@ ** ****************************************************************************/ +#include +#include #include +#include #include #include +#include +#include +#include #include #include #include @@ -40,15 +46,27 @@ #include #include #include +#include "matcher-diagnosticcontainer.h" #include "gtest-qt-printing.h" +using ::testing::Contains; +using ::testing::Not; +using ::testing::PrintToString; + +using ::ClangBackEnd::Diagnostic; using ::ClangBackEnd::DiagnosticSet; -using ::ClangBackEnd::TranslationUnit; +using ::ClangBackEnd::DiagnosticContainer; +using ::ClangBackEnd::FixItContainer; using ::ClangBackEnd::ProjectPart; +using ::ClangBackEnd::SourceLocation; +using ::ClangBackEnd::SourceLocationContainer; +using ::ClangBackEnd::TranslationUnit; using ::ClangBackEnd::UnsavedFiles; namespace { +const Utf8String headerFilePath = Utf8StringLiteral(TESTDATA_DIR"/diagnostic_diagnosticset_header.cpp"); + class DiagnosticSet : public ::testing::Test { protected: @@ -60,6 +78,15 @@ protected: projectPart, Utf8StringVector(), translationUnits}; + TranslationUnit translationUnitMainFile{Utf8StringLiteral(TESTDATA_DIR"/diagnostic_diagnosticset_mainfile.cpp"), + projectPart, + Utf8StringVector(), + translationUnits}; + ::DiagnosticSet diagnosticSetWithChildren{translationUnitMainFile.diagnostics()}; + +protected: + enum ChildMode { WithChild, WithoutChild }; + DiagnosticContainer expectedDiagnostic(ChildMode childMode) const; }; TEST_F(DiagnosticSet, SetHasContent) @@ -120,4 +147,63 @@ TEST_F(DiagnosticSet, BeginPlusOneIsEqualEnd) ASSERT_TRUE(++set.begin() == set.end()); } +TEST_F(DiagnosticSet, ToDiagnosticContainersLetThroughByDefault) +{ + const auto diagnosticContainerWithoutChild = expectedDiagnostic(WithChild); + + const auto diagnostics = translationUnitMainFile.diagnostics().toDiagnosticContainers(); + + ASSERT_THAT(diagnostics, Contains(IsDiagnosticContainer(diagnosticContainerWithoutChild))); +} + +TEST_F(DiagnosticSet, ToDiagnosticContainersFiltersOutTopLevelItem) +{ + const auto acceptNoDiagnostics = [](const Diagnostic &) { return false; }; + + const auto diagnostics = diagnosticSetWithChildren.toDiagnosticContainers(acceptNoDiagnostics); + + ASSERT_TRUE(diagnostics.isEmpty()); +} + +TEST_F(DiagnosticSet, ToDiagnosticContainersFiltersOutChildren) +{ + const auto diagnosticContainerWithoutChild = expectedDiagnostic(WithoutChild); + const auto acceptMainFileDiagnostics = [this](const Diagnostic &diagnostic) { + return diagnostic.location().filePath() == translationUnitMainFile.filePath(); + }; + + const auto diagnostics = diagnosticSetWithChildren.toDiagnosticContainers(acceptMainFileDiagnostics); + + ASSERT_THAT(diagnostics, Contains(IsDiagnosticContainer(diagnosticContainerWithoutChild))); +} + +DiagnosticContainer DiagnosticSet::expectedDiagnostic(DiagnosticSet::ChildMode childMode) const +{ + QVector children; + if (childMode == WithChild) { + const auto child = DiagnosticContainer( + Utf8StringLiteral("note: previous definition is here"), + Utf8StringLiteral("Semantic Issue"), + {Utf8String(), Utf8String()}, + ClangBackEnd::DiagnosticSeverity::Note, + SourceLocationContainer(headerFilePath, 1, 5), + {}, + {}, + {} + ); + children.append(child); + } + + return DiagnosticContainer( + Utf8StringLiteral("error: redefinition of 'f'"), + Utf8StringLiteral("Semantic Issue"), + {Utf8String(), Utf8String()}, + ClangBackEnd::DiagnosticSeverity::Error, + SourceLocationContainer(translationUnitMainFile.filePath(), 3, 53), + {}, + {}, + children + ); +} + } diff --git a/tests/unit/unittest/diagnostictest.cpp b/tests/unit/unittest/diagnostictest.cpp index 1129449310..01925a980d 100644 --- a/tests/unit/unittest/diagnostictest.cpp +++ b/tests/unit/unittest/diagnostictest.cpp @@ -29,7 +29,9 @@ ****************************************************************************/ #include +#include #include +#include #include #include #include @@ -43,9 +45,16 @@ #include #include #include + #include "gtest-qt-printing.h" +#include "matcher-diagnosticcontainer.h" + +using ::testing::Contains; +using ::testing::Not; +using ::testing::PrintToString; using ClangBackEnd::DiagnosticSet; +using ClangBackEnd::DiagnosticContainer; using ClangBackEnd::TranslationUnit; using ClangBackEnd::ProjectPart; using ClangBackEnd::UnsavedFiles; @@ -53,7 +62,8 @@ using ClangBackEnd::Diagnostic; using ClangBackEnd::SourceLocation; using ClangBackEnd::DiagnosticSeverity; using ClangBackEnd::TranslationUnits; -using testing::PrintToString; +using ClangBackEnd::FixItContainer; +using ClangBackEnd::SourceLocationContainer; namespace { @@ -87,6 +97,10 @@ protected: translationUnits}; DiagnosticSet diagnosticSet{translationUnit.diagnostics()}; ::Diagnostic diagnostic{diagnosticSet.back()}; + +protected: + enum ChildMode { WithChild, WithoutChild }; + DiagnosticContainer expectedDiagnostic(ChildMode childMode) const; }; TEST_F(Diagnostic, MoveContructor) @@ -152,4 +166,55 @@ TEST_F(Diagnostic, ChildDiagnosticsText) ASSERT_THAT(childDiagnostic.text(), Utf8StringLiteral("note: previous declaration is here")); } +TEST_F(Diagnostic, toDiagnosticContainerLetChildrenThroughByDefault) +{ + const auto diagnosticWithChild = expectedDiagnostic(WithChild); + + const auto diagnostic = diagnosticSet.front().toDiagnosticContainer(); + + ASSERT_THAT(diagnostic, IsDiagnosticContainer(diagnosticWithChild)); +} + +TEST_F(Diagnostic, toDiagnosticContainerFiltersOutChildren) +{ + const auto diagnosticWithoutChild = expectedDiagnostic(WithoutChild); + const auto acceptDiagnosticWithSpecificText = [](const ::Diagnostic &diagnostic) { + return diagnostic.text() != Utf8StringLiteral("note: previous declaration is here"); + }; + + const auto diagnostic = diagnosticSet.front().toDiagnosticContainer(acceptDiagnosticWithSpecificText); + + ASSERT_THAT(diagnostic, IsDiagnosticContainer(diagnosticWithoutChild)); +} + +DiagnosticContainer Diagnostic::expectedDiagnostic(Diagnostic::ChildMode childMode) const +{ + QVector children; + if (childMode == WithChild) { + const auto child = DiagnosticContainer( + Utf8StringLiteral("note: previous declaration is here"), + Utf8StringLiteral("Semantic Issue"), + {Utf8String(), Utf8String()}, + ClangBackEnd::DiagnosticSeverity::Note, + SourceLocationContainer(translationUnit.filePath(), 2, 14), + {}, + {}, + {} + ); + children.append(child); + } + + return + DiagnosticContainer( + Utf8StringLiteral("warning: 'X' is missing exception specification 'noexcept'"), + Utf8StringLiteral("Semantic Issue"), + {Utf8String(), Utf8String()}, + ClangBackEnd::DiagnosticSeverity::Warning, + SourceLocationContainer(translationUnit.filePath(), 5, 38), + {}, + {}, + children + ); +} + } diff --git a/tests/unit/unittest/matcher-diagnosticcontainer.h b/tests/unit/unittest/matcher-diagnosticcontainer.h new file mode 100644 index 0000000000..da29a38be2 --- /dev/null +++ b/tests/unit/unittest/matcher-diagnosticcontainer.h @@ -0,0 +1,63 @@ +/**************************************************************************** +** +** Copyright (C) 2015 The Qt Company Ltd. +** Contact: http://www.qt.io/licensing +** +** This file is part of Qt Creator. +** +** Commercial License Usage +** Licensees holding valid commercial Qt licenses may use this file in +** accordance with the commercial license agreement provided with the +** Software or, alternatively, in accordance with the terms contained in +** a written agreement between you and The Qt Company. For licensing terms and +** conditions see http://www.qt.io/terms-conditions. For further information +** use the contact form at http://www.qt.io/contact-us. +** +** GNU Lesser General Public License Usage +** Alternatively, this file may be used under the terms of the GNU Lesser +** General Public License version 2.1 or version 3 as published by the Free +** Software Foundation and appearing in the file LICENSE.LGPLv21 and +** LICENSE.LGPLv3 included in the packaging of this file. Please review the +** following information to ensure the GNU Lesser General Public License +** requirements will be met: https://www.gnu.org/licenses/lgpl.html and +** http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html. +** +** In addition, as a special exception, The Qt Company gives you certain additional +** rights. These rights are described in The Qt Company LGPL Exception +** version 1.1, included in the file LGPL_EXCEPTION.txt in this package. +** +****************************************************************************/ + +#include +#include +#include +#include "gtest-qt-printing.h" + +namespace { + +using ::testing::PrintToString; + +MATCHER_P(IsDiagnosticContainer, diagnosticContainer, "") +{ + if (arg.text() != diagnosticContainer.text()) { + *result_listener << "text is " + PrintToString(arg.text()) + + " and not " + PrintToString(diagnosticContainer.text()); + return false; + } + + if (arg.location() != diagnosticContainer.location()) { + *result_listener << "location is " + PrintToString(arg.location()) + + " and not " + PrintToString(diagnosticContainer.location()); + return false; + } + + if (arg.children() != diagnosticContainer.children()) { + *result_listener << "children are " + PrintToString(arg.children()) + + " and not " + PrintToString(diagnosticContainer.children()); + return false; + } + + return true; +} + +} // anonymous diff --git a/tests/unit/unittest/unittest.pro b/tests/unit/unittest/unittest.pro index 744c15cb37..193f1caa68 100644 --- a/tests/unit/unittest/unittest.pro +++ b/tests/unit/unittest/unittest.pro @@ -59,6 +59,7 @@ HEADERS += \ gtest-qt-printing.h \ mockipclient.h \ mockipcserver.h \ - spydummy.h + spydummy.h \ + matcher-diagnosticcontainer.h OTHER_FILES += $$files(data/*) -- GitLab