[PATCH] D143974: [clangd] Inactive regions support via dedicated protocol

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D143974#4267320 , @nridge wrote:

> The patch is causing `initialize-params.test` to fail, fix coming right up.

Fixed in 
https://github.com/llvm/llvm-project/commit/28575f41cd7df98012fb15f18434411a941ec228.


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

[PATCH] D143974: [clangd] Inactive regions support via dedicated protocol

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

The patch is causing `initialize-params.test` to fail, fix coming right up.


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

[PATCH] D143974: [clangd] Inactive regions support via dedicated protocol

[Nathan Ridge via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3f6a904b2f3d: [clangd] Inactive regions support via 
dedicated protocol (authored by nridge).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143974/new/

https://reviews.llvm.org/D143974

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -89,7 +89,8 @@
   for (auto File : AdditionalFiles)
 TU.AdditionalFiles.insert({File.first, std::string(File.second)});
   auto AST = TU.build();
-  auto Actual = getSemanticHighlightings(AST);
+  auto Actual =
+  getSemanticHighlightings(AST, /*IncludeInactiveRegionTokens=*/true);
   for (auto  : Actual)
 Token.Modifiers &= ModifierMask;
 
Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -1310,6 +1310,50 @@
   });
   N.wait();
 }
+
+TEST(ClangdServer, InactiveRegions) {
+  struct InactiveRegionsCallback : ClangdServer::Callbacks {
+std::vector> FoundInactiveRegions;
+
+void onInactiveRegionsReady(PathRef FIle,
+std::vector InactiveRegions) override {
+  FoundInactiveRegions.push_back(std::move(InactiveRegions));
+}
+  };
+
+  MockFS FS;
+  MockCompilationDatabase CDB;
+  CDB.ExtraClangFlags.push_back("-DCMDMACRO");
+  auto Opts = ClangdServer::optsForTest();
+  Opts.PublishInactiveRegions = true;
+  InactiveRegionsCallback Callback;
+  ClangdServer Server(CDB, FS, Opts, );
+  Annotations Source(R"cpp(
+#define PREAMBLEMACRO 42
+#if PREAMBLEMACRO > 40
+  #define ACTIVE
+$inactive1[[#else
+  #define INACTIVE
+#endif]]
+int endPreamble;
+$inactive2[[#ifndef CMDMACRO
+int inactiveInt;
+#endif]]
+#undef CMDMACRO
+$inactive3[[#ifdef CMDMACRO
+  int inactiveInt2;
+#else]]
+  int activeInt;
+#endif
+  )cpp");
+  Server.addDocument(testPath("foo.cpp"), Source.code());
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  EXPECT_THAT(Callback.FoundInactiveRegions,
+  ElementsAre(ElementsAre(Source.range("inactive1"),
+  Source.range("inactive2"),
+  Source.range("inactive3";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -344,7 +344,8 @@
 
   void buildSemanticHighlighting(std::optional LineRange) {
 log("Building semantic highlighting");
-auto Highlights = getSemanticHighlightings(*AST);
+auto Highlights =
+getSemanticHighlightings(*AST, /*IncludeInactiveRegionTokens=*/true);
 for (const auto HL : Highlights)
   if (!LineRange || LineRange->contains(HL.R))
 vlog(" {0} {1} {2}", HL.R, HL.Kind, HL.Modifiers);
Index: clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
@@ -47,14 +47,16 @@
 // Now we hit the TUDecl case where commonAncestor() returns null
 // intendedly. We only annotate tokens in the main file, so use the default
 // traversal scope (which is the top level decls of the main file).
-HighlightingTokens = getSemanticHighlightings(*Inputs.AST);
+HighlightingTokens = getSemanticHighlightings(
+*Inputs.AST, /*IncludeInactiveRegionTokens=*/true);
   } else {
 // Store the existing scopes.
 const auto  = Inputs.AST->getASTContext().getTraversalScope();
 // Narrow the traversal scope to the selected node.
 Inputs.AST->getASTContext().setTraversalScope(
 {const_cast(CommonDecl)});
-HighlightingTokens = getSemanticHighlightings(*Inputs.AST);
+HighlightingTokens = getSemanticHighlightings(
+

[PATCH] D143974: [clangd] Inactive regions support via dedicated protocol

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



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

hokein wrote:
> nridge wrote:
> > hokein wrote:
> > > 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. 
> > I think it might make sense to keep this support around for a while, so 
> > that users whose clients do not yet support this extension still have 
> > //some// indication of which preprocessor branches are inactive.
> > 
> > However, we could make it optional, since some users (for example those who 
> > have to work on large sections of code in inactive preprocessor branches) 
> > may prefer to see the client-side colors over having it all highlighted as 
> > one color.
> Rendering the inactive code as comment is the best we can perform per the 
> standard LSP spec, but it provides an suboptimal (possibly confusing) UX 
> experience, we have received some user complains about that.
> 
> Since we are doing it via an extension, I think we should just remove the old 
> one in the next release (17).
> I think we should just remove the old one in the next release (17).

To give other clients a bit more time to implement the extension, perhaps we 
could:

 * in v17: disable the comment tokens by default, but still have an option to 
turn them on
 * in v18: remove it altogether?

This way, if someone using a non-vscode client has upgraded to clangd 17, but 
their client hasn't implemented the protocol extension yet, and they prefer to 
see the comment tokens (to know which branches are inactive), they can turn 
them on.


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

[PATCH] D143974: [clangd] Inactive regions support via dedicated protocol

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 513450.
nridge marked 2 inline comments as done.
nridge added a comment.

address nit


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143974/new/

https://reviews.llvm.org/D143974

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -89,7 +89,8 @@
   for (auto File : AdditionalFiles)
 TU.AdditionalFiles.insert({File.first, std::string(File.second)});
   auto AST = TU.build();
-  auto Actual = getSemanticHighlightings(AST);
+  auto Actual =
+  getSemanticHighlightings(AST, /*IncludeInactiveRegionTokens=*/true);
   for (auto  : Actual)
 Token.Modifiers &= ModifierMask;
 
Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -1310,6 +1310,50 @@
   });
   N.wait();
 }
+
+TEST(ClangdServer, InactiveRegions) {
+  struct InactiveRegionsCallback : ClangdServer::Callbacks {
+std::vector> FoundInactiveRegions;
+
+void onInactiveRegionsReady(PathRef FIle,
+std::vector InactiveRegions) override {
+  FoundInactiveRegions.push_back(std::move(InactiveRegions));
+}
+  };
+
+  MockFS FS;
+  MockCompilationDatabase CDB;
+  CDB.ExtraClangFlags.push_back("-DCMDMACRO");
+  auto Opts = ClangdServer::optsForTest();
+  Opts.PublishInactiveRegions = true;
+  InactiveRegionsCallback Callback;
+  ClangdServer Server(CDB, FS, Opts, );
+  Annotations Source(R"cpp(
+#define PREAMBLEMACRO 42
+#if PREAMBLEMACRO > 40
+  #define ACTIVE
+$inactive1[[#else
+  #define INACTIVE
+#endif]]
+int endPreamble;
+$inactive2[[#ifndef CMDMACRO
+int inactiveInt;
+#endif]]
+#undef CMDMACRO
+$inactive3[[#ifdef CMDMACRO
+  int inactiveInt2;
+#else]]
+  int activeInt;
+#endif
+  )cpp");
+  Server.addDocument(testPath("foo.cpp"), Source.code());
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  EXPECT_THAT(Callback.FoundInactiveRegions,
+  ElementsAre(ElementsAre(Source.range("inactive1"),
+  Source.range("inactive2"),
+  Source.range("inactive3";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -344,7 +344,8 @@
 
   void buildSemanticHighlighting(std::optional LineRange) {
 log("Building semantic highlighting");
-auto Highlights = getSemanticHighlightings(*AST);
+auto Highlights =
+getSemanticHighlightings(*AST, /*IncludeInactiveRegionTokens=*/true);
 for (const auto HL : Highlights)
   if (!LineRange || LineRange->contains(HL.R))
 vlog(" {0} {1} {2}", HL.R, HL.Kind, HL.Modifiers);
Index: clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
@@ -47,14 +47,16 @@
 // Now we hit the TUDecl case where commonAncestor() returns null
 // intendedly. We only annotate tokens in the main file, so use the default
 // traversal scope (which is the top level decls of the main file).
-HighlightingTokens = getSemanticHighlightings(*Inputs.AST);
+HighlightingTokens = getSemanticHighlightings(
+*Inputs.AST, /*IncludeInactiveRegionTokens=*/true);
   } else {
 // Store the existing scopes.
 const auto  = Inputs.AST->getASTContext().getTraversalScope();
 // Narrow the traversal scope to the selected node.
 Inputs.AST->getASTContext().setTraversalScope(
 {const_cast(CommonDecl)});
-HighlightingTokens = getSemanticHighlightings(*Inputs.AST);
+HighlightingTokens = getSemanticHighlightings(
+*Inputs.AST, /*IncludeInactiveRegionTokens=*/true);
 // Restore the traversal scope.
 

[PATCH] D143974: [clangd] Inactive regions support via dedicated protocol

[Haojian Wu via Phabricator via cfe-commits]

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

Thanks, the implementation looks good.




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

nridge wrote:
> hokein wrote:
> > 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. 
> I think it might make sense to keep this support around for a while, so that 
> users whose clients do not yet support this extension still have //some// 
> indication of which preprocessor branches are inactive.
> 
> However, we could make it optional, since some users (for example those who 
> have to work on large sections of code in inactive preprocessor branches) may 
> prefer to see the client-side colors over having it all highlighted as one 
> color.
Rendering the inactive code as comment is the best we can perform per the 
standard LSP spec, but it provides an suboptimal (possibly confusing) UX 
experience, we have received some user complains about that.

Since we are doing it via an extension, I think we should just remove the old 
one in the next release (17).



Comment at: clang-tools-extra/clangd/Protocol.h:1743
 
+/// Parameters for the inactive regions (server-side) push notification.
+struct InactiveRegionsParams {

nit: mention this is a clangd extension as well.


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

[PATCH] D143974: [clangd] Inactive regions support via dedicated protocol

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked an inline comment as done.
nridge added inline comments.



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

hokein wrote:
> 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. 
I think it might make sense to keep this support around for a while, so that 
users whose clients do not yet support this extension still have //some// 
indication of which preprocessor branches are inactive.

However, we could make it optional, since some users (for example those who 
have to work on large sections of code in inactive preprocessor branches) may 
prefer to see the client-side colors over having it all highlighted as one 
color.



Comment at: clang-tools-extra/clangd/ClangdServer.h:91
+virtual void onInactiveRegionsReady(PathRef File,
+std::vector InactiveRegions) {}
   };

hokein wrote:
> 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`).
Good idea -- added!


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

[PATCH] D143974: [clangd] Inactive regions support via dedicated protocol

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 511957.
nridge marked 3 inline comments as done.
nridge added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143974/new/

https://reviews.llvm.org/D143974

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -89,7 +89,8 @@
   for (auto File : AdditionalFiles)
 TU.AdditionalFiles.insert({File.first, std::string(File.second)});
   auto AST = TU.build();
-  auto Actual = getSemanticHighlightings(AST);
+  auto Actual =
+  getSemanticHighlightings(AST, /*IncludeInactiveRegionTokens=*/true);
   for (auto  : Actual)
 Token.Modifiers &= ModifierMask;
 
Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -1310,6 +1310,50 @@
   });
   N.wait();
 }
+
+TEST(ClangdServer, InactiveRegions) {
+  struct InactiveRegionsCallback : ClangdServer::Callbacks {
+std::vector> FoundInactiveRegions;
+
+void onInactiveRegionsReady(PathRef FIle,
+std::vector InactiveRegions) override {
+  FoundInactiveRegions.push_back(std::move(InactiveRegions));
+}
+  };
+
+  MockFS FS;
+  MockCompilationDatabase CDB;
+  CDB.ExtraClangFlags.push_back("-DCMDMACRO");
+  auto Opts = ClangdServer::optsForTest();
+  Opts.PublishInactiveRegions = true;
+  InactiveRegionsCallback Callback;
+  ClangdServer Server(CDB, FS, Opts, );
+  Annotations Source(R"cpp(
+#define PREAMBLEMACRO 42
+#if PREAMBLEMACRO > 40
+  #define ACTIVE
+$inactive1[[#else
+  #define INACTIVE
+#endif]]
+int endPreamble;
+$inactive2[[#ifndef CMDMACRO
+int inactiveInt;
+#endif]]
+#undef CMDMACRO
+$inactive3[[#ifdef CMDMACRO
+  int inactiveInt2;
+#else]]
+  int activeInt;
+#endif
+  )cpp");
+  Server.addDocument(testPath("foo.cpp"), Source.code());
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+  EXPECT_THAT(Callback.FoundInactiveRegions,
+  ElementsAre(ElementsAre(Source.range("inactive1"),
+  Source.range("inactive2"),
+  Source.range("inactive3";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -344,7 +344,8 @@
 
   void buildSemanticHighlighting(std::optional LineRange) {
 log("Building semantic highlighting");
-auto Highlights = getSemanticHighlightings(*AST);
+auto Highlights =
+getSemanticHighlightings(*AST, /*IncludeInactiveRegionTokens=*/true);
 for (const auto HL : Highlights)
   if (!LineRange || LineRange->contains(HL.R))
 vlog(" {0} {1} {2}", HL.R, HL.Kind, HL.Modifiers);
Index: clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
@@ -47,14 +47,16 @@
 // Now we hit the TUDecl case where commonAncestor() returns null
 // intendedly. We only annotate tokens in the main file, so use the default
 // traversal scope (which is the top level decls of the main file).
-HighlightingTokens = getSemanticHighlightings(*Inputs.AST);
+HighlightingTokens = getSemanticHighlightings(
+*Inputs.AST, /*IncludeInactiveRegionTokens=*/true);
   } else {
 // Store the existing scopes.
 const auto  = Inputs.AST->getASTContext().getTraversalScope();
 // Narrow the traversal scope to the selected node.
 Inputs.AST->getASTContext().setTraversalScope(
 {const_cast(CommonDecl)});
-HighlightingTokens = getSemanticHighlightings(*Inputs.AST);
+HighlightingTokens = getSemanticHighlightings(
+*Inputs.AST, /*IncludeInactiveRegionTokens=*/true);
 // Restore the traversal scope.
 

[PATCH] D143974: [clangd] Inactive regions support via dedicated protocol

[Haojian Wu via Phabricator via cfe-commits]

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 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

[PATCH] D143974: [clangd] Inactive regions support via dedicated protocol

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

The issue addressed by this patch is attracting a steady stream of user 
interest (two recent examples are https://github.com/clangd/clangd/issues/1545 
and https://github.com/clangd/vscode-clangd/issues/469).

A review would be appreciated :)


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

[PATCH] D143974: [clangd] Inactive regions support via dedicated protocol

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Corresponding client-side support is implemented for vscode in the latest 
version of https://github.com/clangd/vscode-clangd/pull/193.


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

[PATCH] D143974: [clangd] Inactive regions support via dedicated protocol

[Nathan Ridge via Phabricator via cfe-commits]

nridge created this revision.
nridge added reviewers: sammccall, kadircet, hokein.
Herald added a subscriber: arphaman.
Herald added a project: All.
nridge requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

This implements the server side of the approach discussed at
https://github.com/clangd/vscode-clangd/pull/193#issuecomment-1044315732


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143974

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -89,7 +89,8 @@
   for (auto File : AdditionalFiles)
 TU.AdditionalFiles.insert({File.first, std::string(File.second)});
   auto AST = TU.build();
-  auto Actual = getSemanticHighlightings(AST);
+  auto Actual =
+  getSemanticHighlightings(AST, /*IncludeInactiveRegionTokens=*/true);
   for (auto  : Actual)
 Token.Modifiers &= ModifierMask;
 
Index: clang-tools-extra/clangd/tool/Check.cpp
===
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -344,7 +344,8 @@
 
   void buildSemanticHighlighting(std::optional LineRange) {
 log("Building semantic highlighting");
-auto Highlights = getSemanticHighlightings(*AST);
+auto Highlights =
+getSemanticHighlightings(*AST, /*IncludeInactiveRegionTokens=*/true);
 for (const auto HL : Highlights)
   if (!LineRange || LineRange->contains(HL.R))
 vlog(" {0} {1} {2}", HL.R, HL.Kind, HL.Modifiers);
Index: clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
===
--- clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
+++ clang-tools-extra/clangd/refactor/tweaks/AnnotateHighlightings.cpp
@@ -47,14 +47,16 @@
 // Now we hit the TUDecl case where commonAncestor() returns null
 // intendedly. We only annotate tokens in the main file, so use the default
 // traversal scope (which is the top level decls of the main file).
-HighlightingTokens = getSemanticHighlightings(*Inputs.AST);
+HighlightingTokens = getSemanticHighlightings(
+*Inputs.AST, /*IncludeInactiveRegionTokens=*/true);
   } else {
 // Store the existing scopes.
 const auto  = Inputs.AST->getASTContext().getTraversalScope();
 // Narrow the traversal scope to the selected node.
 Inputs.AST->getASTContext().setTraversalScope(
 {const_cast(CommonDecl)});
-HighlightingTokens = getSemanticHighlightings(*Inputs.AST);
+HighlightingTokens = getSemanticHighlightings(
+*Inputs.AST, /*IncludeInactiveRegionTokens=*/true);
 // Restore the traversal scope.
 Inputs.AST->getASTContext().setTraversalScope(BackupScopes);
   }
Index: clang-tools-extra/clangd/SemanticHighlighting.h
===
--- clang-tools-extra/clangd/SemanticHighlighting.h
+++ clang-tools-extra/clangd/SemanticHighlighting.h
@@ -105,7 +105,8 @@
 
 // Returns all HighlightingTokens from an AST. Only generates highlights for the
 // main AST.
-std::vector getSemanticHighlightings(ParsedAST );
+std::vector
+getSemanticHighlightings(ParsedAST , bool IncludeInactiveRegionTokens);
 
 std::vector toSemanticTokens(llvm::ArrayRef,
 llvm::StringRef Code);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -357,9 +357,10 @@
 /// Consumes source locations and maps them to text ranges for highlightings.
 class HighlightingsBuilder {
 public:
-  HighlightingsBuilder(const ParsedAST )
+  HighlightingsBuilder(const ParsedAST , bool IncludeInactiveRegionTokens)
   : TB(AST.getTokens()), SourceMgr(AST.getSourceManager()),
-LangOpts(AST.getLangOpts()) {}
+LangOpts(AST.getLangOpts()),
+IncludeInactiveRegionTokens(IncludeInactiveRegionTokens) {}
 
   HighlightingToken (SourceLocation Loc, HighlightingKind Kind) {
 auto Range = getRangeForSourceLocation(Loc);
@@