[PATCH] D40860: [clangd] Fix diagnostic positions

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Dec 11 08:09:13 PST 2017

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.


More information about the cfe-commits mailing list