subject:"\[PATCH\] D60822\: \[clangd\] Use shorter, more recognizable codes for diagnostics."

[PATCH] D60822: [clangd] Use shorter, more recognizable codes for diagnostics.

2019-04-17 Thread Sam McCall via Phabricator via cfe-commits

sammccall added a comment.

In D60822#1470227 , @kadircet wrote:

> I agree that these are more useful. What about also adding "-W" in front of 
> warning flags to make them more explicit and stand out from "non-flag" error 
> names?

I went back and forth on this, I think you're probably right. It's consistent 
with how clang-tidy check names vs warnings are exposed to users: "Diag text 
[-Wfoo]" and "Diag text [check-name]".
Let's see how it goes.

Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60822/new/

https://reviews.llvm.org/D60822

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60822: [clangd] Use shorter, more recognizable codes for diagnostics.

2019-04-17 Thread Sam McCall via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE358611: [clangd] Use shorter, more recognizable codes for 
diagnostics. (authored by sammccall, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D60822?vs=195623=195624#toc

Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60822/new/

https://reviews.llvm.org/D60822

Files:
  clangd/Diagnostics.cpp
  test/clangd/compile-commands-path-in-initialize.test
  test/clangd/diagnostic-category.test
  test/clangd/diagnostics.test
  test/clangd/did-change-configuration-params.test
  test/clangd/execute-command.test
  test/clangd/fixits-codeaction.test
  test/clangd/fixits-command.test
  test/clangd/fixits-embed-in-diagnostic.test
  unittests/clangd/DiagnosticsTests.cpp

Index: clangd/Diagnostics.cpp
===
--- clangd/Diagnostics.cpp
+++ clangd/Diagnostics.cpp
@@ -331,7 +331,17 @@
   // Fill in name/source now that we have all the context needed to map them.
   for (auto  : Output) {
 if (const char *ClangDiag = getDiagnosticCode(Diag.ID)) {
-  Diag.Name = ClangDiag;
+  // Warnings controlled by -Wfoo are better recognized by that name.
+  StringRef Warning = DiagnosticIDs::getWarningOptionForDiag(Diag.ID);
+  if (!Warning.empty()) {
+Diag.Name = ("-W" + Warning).str();
+  } else {
+StringRef Name(ClangDiag);
+// Almost always an error, with a name like err_enum_class_reference.
+// Drop the err_ prefix for brevity.
+Name.consume_front("err_");
+Diag.Name = Name;
+  }
   Diag.Source = Diag::Clang;
   continue;
 }
Index: test/clangd/fixits-codeaction.test
===
--- test/clangd/fixits-codeaction.test
+++ test/clangd/fixits-codeaction.test
@@ -6,7 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"code": "warn_condition_is_assignment",
+# CHECK-NEXT:"code": "-Wparentheses",
 # CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
@@ -25,14 +25,14 @@
 # CHECK-NEXT:"uri": "file://{{.*}}/foo.c"
 # CHECK-NEXT:  }
 ---
-{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses (fixes available)", "code": "warn_condition_is_assignment", "source": "clang"}]}}}
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses (fixes available)", "code": "-Wparentheses", "source": "clang"}]}}}
 #  CHECK:  "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": [
 # CHECK-NEXT:{
 # CHECK-NEXT:  "diagnostics": [
 # CHECK-NEXT:{
-# CHECK-NEXT:  "code": "warn_condition_is_assignment",
+# CHECK-NEXT:  "code": "-Wparentheses",
 # CHECK-NEXT:  "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:  "range": {
 # CHECK-NEXT:"end": {
@@ -86,7 +86,7 @@
 # CHECK-NEXT:{
 # CHECK-NEXT:  "diagnostics": [
 # CHECK-NEXT:{
-# CHECK-NEXT:  "code": "warn_condition_is_assignment",
+# CHECK-NEXT:  "code": "-Wparentheses",
 # CHECK-NEXT:  "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:  "range": {
 # CHECK-NEXT:"end": {
Index: test/clangd/did-change-configuration-params.test
===
--- test/clangd/did-change-configuration-params.test
+++ test/clangd/did-change-configuration-params.test
@@ -24,7 +24,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"code": "warn_uninit_var",
+# CHECK-NEXT:"code": "-Wuninitialized",
 # CHECK-NEXT:"message": "Variable 'i' is uninitialized when used here (fix available)",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/diagnostic-category.test
===
--- test/clangd/diagnostic-category.test
+++

[PATCH] D60822: [clangd] Use shorter, more recognizable codes for diagnostics.

2019-04-17 Thread Sam McCall via Phabricator via cfe-commits

sammccall updated this revision to Diff 195623.
sammccall added a comment.

-Wfoo instead of foo


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60822/new/

https://reviews.llvm.org/D60822

Files:
  clangd/Diagnostics.cpp
  test/clangd/compile-commands-path-in-initialize.test
  test/clangd/diagnostic-category.test
  test/clangd/diagnostics.test
  test/clangd/did-change-configuration-params.test
  test/clangd/execute-command.test
  test/clangd/fixits-codeaction.test
  test/clangd/fixits-command.test
  test/clangd/fixits-embed-in-diagnostic.test
  unittests/clangd/DiagnosticsTests.cpp

Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -107,7 +107,7 @@
   AllOf(Diag(Test.range("typo"),
  "use of undeclared identifier 'goo'; did you mean 'foo'?"),
 DiagSource(Diag::Clang),
-DiagName("err_undeclared_var_use_suggest"),
+DiagName("undeclared_var_use_suggest"),
 WithFix(
 Fix(Test.range("typo"), "foo", "change 'go\\ o' to 'foo'")),
 // This is a pretty normal range.
@@ -152,7 +152,7 @@
   EXPECT_THAT(TU.build().getDiagnostics(),
   ElementsAre(testing::AllOf(
   Diag(Test.range(), "'not-found.h' file not found"),
-  DiagSource(Diag::Clang), DiagName("err_pp_file_not_found";
+  DiagSource(Diag::Clang), DiagName("pp_file_not_found";
 }
 
 TEST(DiagnosticsTest, ClangTidy) {
@@ -252,7 +252,7 @@
 TEST(DiagnosticsTest, ToLSP) {
   clangd::Diag D;
   D.ID = clang::diag::err_enum_class_reference;
-  D.Name = "err_enum_class_reference";
+  D.Name = "enum_class_reference";
   D.Source = clangd::Diag::Clang;
   D.Message = "something terrible happened";
   D.Range = {pos(1, 2), pos(3, 4)};
@@ -322,7 +322,7 @@
   LSPDiags,
   ElementsAre(Pair(EqualToLSPDiag(MainLSP), ElementsAre(EqualToFix(F))),
   Pair(EqualToLSPDiag(NoteInMainLSP), IsEmpty(;
-  EXPECT_EQ(LSPDiags[0].first.code, "err_enum_class_reference");
+  EXPECT_EQ(LSPDiags[0].first.code, "enum_class_reference");
   EXPECT_EQ(LSPDiags[0].first.source, "clang");
   EXPECT_EQ(LSPDiags[1].first.code, "");
   EXPECT_EQ(LSPDiags[1].first.source, "");
Index: test/clangd/fixits-embed-in-diagnostic.test
===
--- test/clangd/fixits-embed-in-diagnostic.test
+++ test/clangd/fixits-embed-in-diagnostic.test
@@ -6,7 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"code": "err_use_with_wrong_tag",
+# CHECK-NEXT:"code": "use_with_wrong_tag",
 # CHECK-NEXT:"codeActions": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"edit": {
Index: test/clangd/fixits-command.test
===
--- test/clangd/fixits-command.test
+++ test/clangd/fixits-command.test
@@ -6,7 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"code": "warn_condition_is_assignment",
+# CHECK-NEXT:"code": "-Wparentheses",
 # CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/fixits-codeaction.test
===
--- test/clangd/fixits-codeaction.test
+++ test/clangd/fixits-codeaction.test
@@ -6,7 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"code": "warn_condition_is_assignment",
+# CHECK-NEXT:"code": "-Wparentheses",
 # CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
@@ -25,14 +25,14 @@
 # CHECK-NEXT:"uri": "file://{{.*}}/foo.c"
 # CHECK-NEXT:  }
 ---
-{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses (fixes available)", "code": "warn_condition_is_assignment", "source": "clang"}]}}}
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character":

[PATCH] D60822: [clangd] Use shorter, more recognizable codes for diagnostics.

2019-04-17 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

I agree that these are more useful. What about also adding "-W" in front of 
warning flags to make them more explicit and stand out from "non-flag" error 
names?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60822/new/

https://reviews.llvm.org/D60822



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60822: [clangd] Use shorter, more recognizable codes for diagnostics.

2019-04-17 Thread Sam McCall via Phabricator via cfe-commits

sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ioeric, 
ilya-biryukov.
Herald added a project: clang.

- for warnings, use "foo" if the warning is controlled by -Wfoo
- for errors, keep using the internal name (there's nothing better) but drop 
the err_ prefix

This comes at the cost of uniformity, it's no longer totally obvious
exactly what the code field contains. But the -Wname flags are so much
more useful to end-users than the internal warn_foo that this seems worth it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D60822

Files:
  clangd/Diagnostics.cpp
  test/clangd/compile-commands-path-in-initialize.test
  test/clangd/diagnostic-category.test
  test/clangd/diagnostics.test
  test/clangd/did-change-configuration-params.test
  test/clangd/execute-command.test
  test/clangd/fixits-codeaction.test
  test/clangd/fixits-command.test
  test/clangd/fixits-embed-in-diagnostic.test
  unittests/clangd/DiagnosticsTests.cpp

Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -107,7 +107,7 @@
   AllOf(Diag(Test.range("typo"),
  "use of undeclared identifier 'goo'; did you mean 'foo'?"),
 DiagSource(Diag::Clang),
-DiagName("err_undeclared_var_use_suggest"),
+DiagName("undeclared_var_use_suggest"),
 WithFix(
 Fix(Test.range("typo"), "foo", "change 'go\\ o' to 'foo'")),
 // This is a pretty normal range.
@@ -152,7 +152,7 @@
   EXPECT_THAT(TU.build().getDiagnostics(),
   ElementsAre(testing::AllOf(
   Diag(Test.range(), "'not-found.h' file not found"),
-  DiagSource(Diag::Clang), DiagName("err_pp_file_not_found";
+  DiagSource(Diag::Clang), DiagName("pp_file_not_found";
 }
 
 TEST(DiagnosticsTest, ClangTidy) {
@@ -252,7 +252,7 @@
 TEST(DiagnosticsTest, ToLSP) {
   clangd::Diag D;
   D.ID = clang::diag::err_enum_class_reference;
-  D.Name = "err_enum_class_reference";
+  D.Name = "enum_class_reference";
   D.Source = clangd::Diag::Clang;
   D.Message = "something terrible happened";
   D.Range = {pos(1, 2), pos(3, 4)};
@@ -322,7 +322,7 @@
   LSPDiags,
   ElementsAre(Pair(EqualToLSPDiag(MainLSP), ElementsAre(EqualToFix(F))),
   Pair(EqualToLSPDiag(NoteInMainLSP), IsEmpty(;
-  EXPECT_EQ(LSPDiags[0].first.code, "err_enum_class_reference");
+  EXPECT_EQ(LSPDiags[0].first.code, "enum_class_reference");
   EXPECT_EQ(LSPDiags[0].first.source, "clang");
   EXPECT_EQ(LSPDiags[1].first.code, "");
   EXPECT_EQ(LSPDiags[1].first.source, "");
Index: test/clangd/fixits-embed-in-diagnostic.test
===
--- test/clangd/fixits-embed-in-diagnostic.test
+++ test/clangd/fixits-embed-in-diagnostic.test
@@ -6,7 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"code": "err_use_with_wrong_tag",
+# CHECK-NEXT:"code": "use_with_wrong_tag",
 # CHECK-NEXT:"codeActions": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"edit": {
Index: test/clangd/fixits-command.test
===
--- test/clangd/fixits-command.test
+++ test/clangd/fixits-command.test
@@ -6,7 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"code": "warn_condition_is_assignment",
+# CHECK-NEXT:"code": "parentheses",
 # CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/fixits-codeaction.test
===
--- test/clangd/fixits-codeaction.test
+++ test/clangd/fixits-codeaction.test
@@ -6,7 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"code": "warn_condition_is_assignment",
+# CHECK-NEXT:"code": "parentheses",
 # CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
@@ -25,14 +25,14 @@
 # CHECK-NEXT:"uri": "file://{{.*}}/foo.c"
 # CHECK-NEXT:  }
 ---
-{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without

[PATCH] D60822: [clangd] Use shorter, more recognizable codes for diagnostics.

[PATCH] D60822: [clangd] Use shorter, more recognizable codes for diagnostics.

[PATCH] D60822: [clangd] Use shorter, more recognizable codes for diagnostics.

[PATCH] D60822: [clangd] Use shorter, more recognizable codes for diagnostics.

[PATCH] D60822: [clangd] Use shorter, more recognizable codes for diagnostics.

5 matches

Site Navigation

Mail list logo

Footer information