[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 &SrcMgr,
+                                   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 &Location) {
+  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 &SrcMgr,
+                                   const LangOptions &Options,
----------------
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