[PATCH] D40860: [clangd] Fix diagnostic positions

[Raoul Wols via Phabricator via cfe-commits]

rwols closed this revision.
rwols added a comment.

https://reviews.llvm.org/D41118 fixes both the start positions as well as the 
ranges, let's merge that.


https://reviews.llvm.org/D40860



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

[PATCH] D40860: [clangd] Fix diagnostic positions

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

https://reviews.llvm.org/D41118 adds real (non-empty) ranges for diagnostics.
I've copied all the changes you've made here with some slight tweaks, as they 
were required for that too.

If you like it as-is, probably easiest to land that instead.
But I'm also happy for you to land this and I'll merge. Up to you.


https://reviews.llvm.org/D40860



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

[PATCH] D40860: [clangd] Fix diagnostic positions

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Hmm, I looked into the empty diagnostic ranges, and it overlaps here a lot.

Feel free to go ahead and land this and I can merge, otherwise I can include 
this fix there.
Regardless of how it gets landed, thanks for finding and fixing this, this 
explains a lot of weird behavior!




Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:124
+/// Convert a clang::SourceLocation to a clangd::Position
+Position SourceLocToClangdPosition(const SourceManager ,
+   SourceLocation Location) {

sammccall wrote:
> nit: functions are `lowerCamelCase`.
> Here and below.
nit: consider just `toPosition`, with the location as the first argument

And similarly below. The shorter names and putting the main param first make 
the callsites easier to read I think.



Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:200
 public:
-  StoreDiagsConsumer(std::vector ) : Output(Output) {}
+  StoreDiagsConsumer(const LangOptions ,
+ std::vector )

Actually I think the DiagnosticConsumer interface should pass you the language 
options itself, if you override the lifecycle methods.


https://reviews.llvm.org/D40860



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

[PATCH] D40860: [clangd] Fix diagnostic positions

[Sam McCall via Phabricator via cfe-commits]

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

Sorry for the delay getting this reviewed.
LG, thanks for fixing this! Just style nits.




Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:124
+/// Convert a clang::SourceLocation to a clangd::Position
+Position SourceLocToClangdPosition(const SourceManager ,
+   SourceLocation Location) {

nit: functions are `lowerCamelCase`.
Here and below.



Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:136
+/// Convert a clang::FullSourceLoc to a clangd::Position
+Position SourceLocToClangdPosition(const FullSourceLoc ) {
+  return SourceLocToClangdPosition(Location.getManager(), Location);

only used once and private - inline this for now?



Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:150
+/// Convert a clang::CharSourceRange to a clangd::Range
+Range CharSourceRangeToClangdRange(const SourceManager ,
+   const LangOptions ,

It seems like this could be decomposed into

  - CharSourceRange -> `SourceRange`
  - existing `SourceRangeToClangdRange`

The first probably belongs (or exists) elsewhere in clang, though I can't find 
it - fine to keep it here.



Comment at: clang-tools-extra/test/clangd/diagnostics.test:32
 # CHECK-NEXT:  "end": {
-# CHECK-NEXT:"character": 1,
+# CHECK-NEXT:"character": 0,
 # CHECK-NEXT:"line": 0

Incidentally, these tests seem to be wrong! The ranges shouldn't be empty (at 
least this one).

Unrelated to your patch though, I'll look into it separately.


https://reviews.llvm.org/D40860



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

[PATCH] D40860: [clangd] Fix diagnostic positions

[Raoul Wols via Phabricator via cfe-commits]

rwols created this revision.

This translates diagnostics correctly between 0-based LSP columns, and 1-based 
clang columns.


https://reviews.llvm.org/D40860

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdUnit.cpp
  clang-tools-extra/clangd/ClangdUnit.h
  clang-tools-extra/test/clangd/diagnostics.test
  clang-tools-extra/test/clangd/execute-command.test
  clang-tools-extra/test/clangd/extra-flags.test
  clang-tools-extra/test/clangd/fixits.test

Index: clang-tools-extra/test/clangd/fixits.test
===
--- clang-tools-extra/test/clangd/fixits.test
+++ clang-tools-extra/test/clangd/fixits.test
@@ -15,11 +15,11 @@
 # CHECK-NEXT:"message": "using the result of an assignment as a condition without parentheses",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
-# CHECK-NEXT:"character": 35,
+# CHECK-NEXT:"character": 34,
 # CHECK-NEXT:"line": 0
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "start": {
-# CHECK-NEXT:"character": 35,
+# CHECK-NEXT:"character": 34,
 # CHECK-NEXT:"line": 0
 # CHECK-NEXT:  }
 # CHECK-NEXT:},
@@ -29,11 +29,11 @@
 # CHECK-NEXT:"message": "place parentheses around the assignment to silence this warning",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
-# CHECK-NEXT:"character": 35,
+# CHECK-NEXT:"character": 34,
 # CHECK-NEXT:"line": 0
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "start": {
-# CHECK-NEXT:"character": 35,
+# CHECK-NEXT:"character": 34,
 # CHECK-NEXT:"line": 0
 # CHECK-NEXT:  }
 # CHECK-NEXT:},
@@ -43,11 +43,11 @@
 # CHECK-NEXT:"message": "use '==' to turn this assignment into an equality comparison",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
-# CHECK-NEXT:"character": 35,
+# CHECK-NEXT:"character": 34,
 # CHECK-NEXT:"line": 0
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "start": {
-# CHECK-NEXT:"character": 35,
+# CHECK-NEXT:"character": 34,
 # CHECK-NEXT:"line": 0
 # CHECK-NEXT:  }
 # CHECK-NEXT:},
@@ -58,7 +58,7 @@
 # CHECK-NEXT:  }
 Content-Length: 746
 
-{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":34}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 34}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 34}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 34}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}
 #  CHECK:  "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": [
@@ -128,7 +128,7 @@
 # CHECK-NEXT:  ]
 Content-Length: 771
 
-{"jsonrpc":"2.0","id":3,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"code":"1","source":"foo","message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}