hokein added a comment.
Thanks, and sorry for late response.
The patch looks good to me in general.
================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:590
{"foldingRangeProvider", true},
+ {"inactiveRegionsProvider", true},
};
----------------
nit: add trailing `// clangd extension`, probably better to move to below line
585 (since we group all extension there)
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:123
+ ServerCallbacks->onInactiveRegionsReady(Path,
+ std::move(InactiveRegions));
+ }
----------------
nit: just `AST.getMacros().SkppedRanges()`?
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:977
+ return CB(InpAST.takeError());
+ // Include inactive regions in semantic highlighting tokens only if the
+ // client doesn't support a dedicated protocol for being informed about
----------------
As discussed in the GitHub thread (the experiment of highlighting the inactive
regions as comment is considered as a failure), we should always not include
the inactive region in the semantic highlighting, this will also simplify the
existing code in semantic highlighting (e.g.
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/SemanticHighlighting.cpp#L464-L513).
I think we can do it in a separated patch.
================
Comment at: clang-tools-extra/clangd/ClangdServer.h:91
+ virtual void onInactiveRegionsReady(PathRef File,
+ std::vector<Range> InactiveRegions) {}
};
----------------
it would be nice to have a unittest for this, I think it should not be to hard
to add one in `ClangdTests.cpp` (with a customize Callbacks passing to the
`ClangdServer`).
================
Comment at: clang-tools-extra/clangd/Protocol.h:521
+ /// Whether the client supports the textDocument/inactiveRegions
+ /// notificatin. This is a clangd extension.
+ bool InactiveRegions = false;
----------------
nit: notificatin => notification
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143974/new/
https://reviews.llvm.org/D143974
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
