hokein updated this revision to Diff 207760.
hokein added a comment.
Revert the LSPDiagnosticCompare change in rL363889
<https://reviews.llvm.org/rL363889>.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64127/new/
https://reviews.llvm.org/D64127
Files:
clang-tools-extra/clangd/Diagnostics.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/test/fixits-duplication.test
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -179,6 +179,43 @@
DiagSource(Diag::Clang), DiagName("pp_file_not_found"))));
}
+TEST(DiagnosticsTest, DeduplicatedClangTidyDiagnostics) {
+ Annotations Test(R"cpp(
+ float foo = [[0.1f]];
+ )cpp");
+ auto TU = TestTU::withCode(Test.code());
+ // Enable alias clang-tidy checks, these check emits the same diagnostics
+ // (except the check name).
+ TU.ClangTidyChecks = "-*, readability-uppercase-literal-suffix, "
+ "hicpp-uppercase-literal-suffix";
+ // Verify that we filter out the duplicated diagnostic message.
+ EXPECT_THAT(
+ TU.build().getDiagnostics(),
+ UnorderedElementsAre(::testing::AllOf(
+ Diag(Test.range(),
+ "floating point literal has suffix 'f', which is not uppercase"),
+ DiagSource(Diag::ClangTidy))));
+
+ Test = Annotations(R"cpp(
+ template<typename T>
+ void func(T) {
+ float f = [[0.3f]];
+ }
+ void k() {
+ func(123);
+ func(2.0);
+ }
+ )cpp");
+ // The check doesn't handle template instantiations which ends up emitting
+ // duplicated messages, verify that we deduplicate them.
+ EXPECT_THAT(
+ TU.build().getDiagnostics(),
+ UnorderedElementsAre(::testing::AllOf(
+ Diag(Test.range(),
+ "floating point literal has suffix 'f', which is not uppercase"),
+ DiagSource(Diag::ClangTidy))));
+}
+
TEST(DiagnosticsTest, ClangTidy) {
Annotations Test(R"cpp(
#include $deprecated[["assert.h"]]
Index: clang-tools-extra/clangd/test/fixits-duplication.test
===================================================================
--- clang-tools-extra/clangd/test/fixits-duplication.test
+++ /dev/null
@@ -1,221 +0,0 @@
-# RUN: clangd -lit-test -clang-tidy-checks=modernize-use-nullptr,hicpp-use-nullptr -tweaks="" < %s | FileCheck -strict-whitespace %s
-{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"codeAction":{"codeActionLiteralSupport":{}}}},"trace":"off"}}
----
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.cpp","languageId":"cpp","version":1,"text":"void foo() { char* p = 0; }"}}}
-# CHECK: "method": "textDocument/publishDiagnostics",
-# CHECK-NEXT: "params": {
-# CHECK-NEXT: "diagnostics": [
-# CHECK-NEXT: {
-# CHECK-NEXT: "code": "hicpp-use-nullptr",
-# CHECK-NEXT: "message": "Use nullptr (fix available)",
-# CHECK-NEXT: "range": {
-# CHECK-NEXT: "end": {
-# CHECK-NEXT: "character": 24,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: },
-# CHECK-NEXT: "start": {
-# CHECK-NEXT: "character": 23,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: }
-# CHECK-NEXT: },
-# CHECK-NEXT: "severity": 2,
-# CHECK-NEXT: "source": "clang-tidy"
-# CHECK-NEXT: },
-# CHECK-NEXT: {
-# CHECK-NEXT: "code": "modernize-use-nullptr",
-# CHECK-NEXT: "message": "Use nullptr (fix available)",
-# CHECK-NEXT: "range": {
-# CHECK-NEXT: "end": {
-# CHECK-NEXT: "character": 24,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: },
-# CHECK-NEXT: "start": {
-# CHECK-NEXT: "character": 23,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: }
-# CHECK-NEXT: },
-# CHECK-NEXT: "severity": 2,
-# CHECK-NEXT: "source": "clang-tidy"
-# CHECK-NEXT: }
-# CHECK-NEXT: ],
-# CHECK-NEXT: "uri": "file:///{{.*}}/foo.cpp"
-# CHECK-NEXT: }
----
-{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.cpp"},"range":{"start":{"line":0,"character":23},"end":{"line":0,"character":24}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 23}, "end": {"line": 0, "character": 24}},"severity":2,"message":"Use nullptr (fix available)", "code": "hicpp-use-nullptr", "source": "clang-tidy"},{"range":{"start": {"line": 0, "character": 23}, "end": {"line": 0, "character": 24}},"severity":2,"message":"Use nullptr (fix available)", "code": "modernize-use-nullptr", "source": "clang-tidy"}]}}}
-# CHECK: "id": 2,
-# CHECK-NEXT: "jsonrpc": "2.0",
-# CHECK-NEXT: "result": [
-# CHECK-NEXT: {
-# CHECK-NEXT: "diagnostics": [
-# CHECK-NEXT: {
-# CHECK-NEXT: "code": "hicpp-use-nullptr",
-# CHECK-NEXT: "message": "Use nullptr (fix available)",
-# CHECK-NEXT: "range": {
-# CHECK-NEXT: "end": {
-# CHECK-NEXT: "character": 24,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: },
-# CHECK-NEXT: "start": {
-# CHECK-NEXT: "character": 23,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: }
-# CHECK-NEXT: },
-# CHECK-NEXT: "severity": 2,
-# CHECK-NEXT: "source": "clang-tidy"
-# CHECK-NEXT: }
-# CHECK-NEXT: ],
-# CHECK-NEXT: "edit": {
-# CHECK-NEXT: "changes": {
-# CHECK-NEXT: "file://{{.*}}/foo.cpp": [
-# CHECK-NEXT: {
-# CHECK-NEXT: "newText": "nullptr",
-# CHECK-NEXT: "range": {
-# CHECK-NEXT: "end": {
-# CHECK-NEXT: "character": 24,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: },
-# CHECK-NEXT: "start": {
-# CHECK-NEXT: "character": 23,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: }
-# CHECK-NEXT: }
-# CHECK-NEXT: }
-# CHECK-NEXT: ]
-# CHECK-NEXT: }
-# CHECK-NEXT: },
-# CHECK-NEXT: "kind": "quickfix",
-# CHECK-NEXT: "title": "change '0' to 'nullptr'"
-# CHECK-NEXT: },
-# CHECK-NEXT: {
-# CHECK-NEXT: "diagnostics": [
-# CHECK-NEXT: {
-# CHECK-NEXT: "code": "modernize-use-nullptr",
-# CHECK-NEXT: "message": "Use nullptr (fix available)",
-# CHECK-NEXT: "range": {
-# CHECK-NEXT: "end": {
-# CHECK-NEXT: "character": 24,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: },
-# CHECK-NEXT: "start": {
-# CHECK-NEXT: "character": 23,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: }
-# CHECK-NEXT: },
-# CHECK-NEXT: "severity": 2,
-# CHECK-NEXT: "source": "clang-tidy"
-# CHECK-NEXT: }
-# CHECK-NEXT: ],
-# CHECK-NEXT: "edit": {
-# CHECK-NEXT: "changes": {
-# CHECK-NEXT: "file://{{.*}}/foo.cpp": [
-# CHECK-NEXT: {
-# CHECK-NEXT: "newText": "nullptr",
-# CHECK-NEXT: "range": {
-# CHECK-NEXT: "end": {
-# CHECK-NEXT: "character": 24,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: },
-# CHECK-NEXT: "start": {
-# CHECK-NEXT: "character": 23,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: }
-# CHECK-NEXT: }
-# CHECK-NEXT: }
-# CHECK-NEXT: ]
-# CHECK-NEXT: }
-# CHECK-NEXT: },
-# CHECK-NEXT: "kind": "quickfix",
-# CHECK-NEXT: "title": "change '0' to 'nullptr'"
-# CHECK-NEXT: }
-# CHECK-NEXT: ]
----
-{"jsonrpc":"2.0","id":3,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.cpp"},"range":{"start":{"line":0,"character":23},"end":{"line":0,"character":24}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 23}, "end": {"line": 0, "character": 24}},"severity":2,"message":"Use nullptr (fix available)", "source": "clang-tidy"},{"range":{"start": {"line": 0, "character": 23}, "end": {"line": 0, "character": 24}},"severity":2,"message":"Use nullptr (fix available)", "source": "clang-tidy"}]}}}
-# CHECK: "id": 3,
-# CHECK-NEXT: "jsonrpc": "2.0",
-# CHECK-NEXT: "result": [
-# CHECK-NEXT: {
-# CHECK-NEXT: "diagnostics": [
-# CHECK-NEXT: {
-# CHECK-NEXT: "message": "Use nullptr (fix available)",
-# CHECK-NEXT: "range": {
-# CHECK-NEXT: "end": {
-# CHECK-NEXT: "character": 24,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: },
-# CHECK-NEXT: "start": {
-# CHECK-NEXT: "character": 23,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: }
-# CHECK-NEXT: },
-# CHECK-NEXT: "severity": 2,
-# CHECK-NEXT: "source": "clang-tidy"
-# CHECK-NEXT: }
-# CHECK-NEXT: ],
-# CHECK-NEXT: "edit": {
-# CHECK-NEXT: "changes": {
-# CHECK-NEXT: "file://{{.*}}/foo.cpp": [
-# CHECK-NEXT: {
-# CHECK-NEXT: "newText": "nullptr",
-# CHECK-NEXT: "range": {
-# CHECK-NEXT: "end": {
-# CHECK-NEXT: "character": 24,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: },
-# CHECK-NEXT: "start": {
-# CHECK-NEXT: "character": 23,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: }
-# CHECK-NEXT: }
-# CHECK-NEXT: }
-# CHECK-NEXT: ]
-# CHECK-NEXT: }
-# CHECK-NEXT: },
-# CHECK-NEXT: "kind": "quickfix",
-# CHECK-NEXT: "title": "change '0' to 'nullptr'"
-# CHECK-NEXT: },
-# CHECK-NEXT: {
-# CHECK-NEXT: "diagnostics": [
-# CHECK-NEXT: {
-# CHECK-NEXT: "message": "Use nullptr (fix available)",
-# CHECK-NEXT: "range": {
-# CHECK-NEXT: "end": {
-# CHECK-NEXT: "character": 24,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: },
-# CHECK-NEXT: "start": {
-# CHECK-NEXT: "character": 23,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: }
-# CHECK-NEXT: },
-# CHECK-NEXT: "severity": 2,
-# CHECK-NEXT: "source": "clang-tidy"
-# CHECK-NEXT: }
-# CHECK-NEXT: ],
-# CHECK-NEXT: "edit": {
-# CHECK-NEXT: "changes": {
-# CHECK-NEXT: "file://{{.*}}/foo.cpp": [
-# CHECK-NEXT: {
-# CHECK-NEXT: "newText": "nullptr",
-# CHECK-NEXT: "range": {
-# CHECK-NEXT: "end": {
-# CHECK-NEXT: "character": 24,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: },
-# CHECK-NEXT: "start": {
-# CHECK-NEXT: "character": 23,
-# CHECK-NEXT: "line": 0
-# CHECK-NEXT: }
-# CHECK-NEXT: }
-# CHECK-NEXT: }
-# CHECK-NEXT: ]
-# CHECK-NEXT: }
-# CHECK-NEXT: },
-# CHECK-NEXT: "kind": "quickfix",
-# CHECK-NEXT: "title": "change '0' to 'nullptr'"
-# CHECK-NEXT: }
-# CHECK-NEXT: ]
----
-{"jsonrpc":"2.0","id":4,"method":"shutdown"}
----
-{"jsonrpc":"2.0","method":"exit"}
-
Index: clang-tools-extra/clangd/Protocol.h
===================================================================
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -665,17 +665,11 @@
/// A LSP-specific comparator used to find diagnostic in a container like
/// std:map.
-/// We only use as many fields of Diagnostic as is needed to make distinct
-/// diagnostics unique in practice, to avoid regression issues from LSP clients
-/// (e.g. VScode), see https://git.io/vbr29
+/// We only use the required fields of Diagnostic to do the comparsion to avoid
+/// any regression issues from LSP clients (e.g. VScode), see
+/// https://git.io/vbr29
struct LSPDiagnosticCompare {
bool operator()(const Diagnostic &LHS, const Diagnostic &RHS) const {
- if (!LHS.code.empty() && !RHS.code.empty()) {
- // If the code is known for both, use the code to diambiguate, as e.g.
- // two checkers could produce diagnostics with the same range and message.
- return std::tie(LHS.range, LHS.message, LHS.code) <
- std::tie(RHS.range, RHS.message, RHS.code);
- }
return std::tie(LHS.range, LHS.message) < std::tie(RHS.range, RHS.message);
}
};
Index: clang-tools-extra/clangd/Diagnostics.cpp
===================================================================
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -424,6 +424,23 @@
}
}
}
+ // Deduplicate clang-tidy diagnostics -- some clang-tidy checks may emit
+ // duplicated messages due to various reasons (e.g. the check doesn't handle
+ // template instantiations well; clang-tidy alias checks).
+ auto ClangTidyDiagsBegin =
+ std::partition(Output.begin(), Output.end(),
+ [](const Diag &D) { return D.Source != Diag::ClangTidy; });
+ auto ClangTidyDiagsEnd = Output.end();
+ llvm::sort(ClangTidyDiagsBegin, ClangTidyDiagsEnd, [](const Diag &LHS,
+ const Diag &RHS) {
+ return std::tie(LHS.Range, LHS.Message) < std::tie(RHS.Range, RHS.Message);
+ });
+ auto Last = std::unique(ClangTidyDiagsBegin, ClangTidyDiagsEnd,
+ [](const Diag &LHS, const Diag &RHS) {
+ return std::tie(LHS.Range, LHS.Message) ==
+ std::tie(RHS.Range, RHS.Message);
+ });
+ Output.erase(Last, ClangTidyDiagsEnd);
return std::move(Output);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
