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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
