[PATCH] D67537: [clangd] Client-side support for inactive regions

[Nathan Ridge via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:193
+  if (scopes[0] == "meta.disabled") {
+this.inactiveDecorationIndex = index;
+return vscode.window.createTextEditorDecorationType({

hokein wrote:
> hmm, looks like clangd 
> [omits](https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clangd/SemanticHighlighting.cpp#L454)
>  the `InactiveCode` token.
> 
> I'd suggest we send it like other tokens, that will simplify the patch, we 
> don't need the `inactiveDecorationIndex` and `isInactive`, I think it also 
> helps when we migrate to the official implementation. WDYT?
> 
> 
This was discussed fairly extensively in the server-side review 
(https://reviews.llvm.org/D67536).

In the end, we decided to go with this approach of having a separate field in 
the protocol for inactive lines, for a variety of reasons including the one 
discussed in https://reviews.llvm.org/D67536#inline-616346


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67537



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67537: [clangd] Client-side support for inactive regions

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 247077.
nridge added a comment.

Update to use 'clangd.preprocessor.inactive' as the scope name for inactive 
lines


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67537

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
  
clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
  clang-tools-extra/clangd/test/semantic-highlighting.test

Index: clang-tools-extra/clangd/test/semantic-highlighting.test
===
--- clang-tools-extra/clangd/test/semantic-highlighting.test
+++ clang-tools-extra/clangd/test/semantic-highlighting.test
@@ -62,7 +62,7 @@
 # CHECK-NEXT:"entity.name.function.preprocessor.cpp"
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
-# CHECK-NEXT:"meta.disabled"
+# CHECK-NEXT:"clangd.preprocessor.inactive"
 # CHECK-NEXT:  ]
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
@@ -63,7 +63,8 @@
   test('Colorizer groups decorations correctly', async () => {
 const scopeTable = [
   [ 'variable' ], [ 'entity.type.function' ],
-  [ 'entity.type.function.method' ]
+  [ 'entity.type.function.method' ],
+  [ 'clangd.preprocessor.inactive' ]
 ];
 // Create the scope source ranges the highlightings should be highlighted
 // at. Assumes the scopes used are the ones in the "scopeTable" variable.
@@ -80,6 +81,12 @@
   new vscode.Position(line.line,
   token.character + token.length)));
 });
+if (line.isInactive) {
+  scopeRanges[scopeRanges.length - 1].push(new vscode.Range(
+new vscode.Position(line.line, 0),
+new vscode.Position(line.line, 0)
+  ));
+}
   });
   return scopeRanges;
 };
@@ -121,7 +128,8 @@
 tokens : [
   {character : 1, length : 2, scopeIndex : 1},
   {character : 10, length : 2, scopeIndex : 2},
-]
+],
+isInactive: false
   },
   {
 line : 2,
@@ -129,7 +137,8 @@
   {character : 3, length : 2, scopeIndex : 1},
   {character : 6, length : 2, scopeIndex : 1},
   {character : 8, length : 2, scopeIndex : 2},
-]
+],
+isInactive: true
   },
 ];
 
@@ -144,7 +153,8 @@
   line : 1,
   tokens : [
 {character : 2, length : 1, scopeIndex : 0},
-  ]
+  ],
+  isInactive: false
 };
 highlighter.highlight(fileUri2, [ highlightingsInLine1 ]);
 assert.deepEqual(
Index: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
@@ -23,6 +23,8 @@
   // with its start position, length and the "lookup table" index of of the
   // semantic highlighting Text Mate scopes.
   tokens?: string;
+  // Whether the line is part of an inactive preprocessor branch.
+  isInactive?: boolean;
 }
 
 // A SemanticHighlightingToken decoded from the base64 data sent by clangd.
@@ -40,6 +42,8 @@
   line: number;
   // All SemanticHighlightingTokens on the line.
   tokens: SemanticHighlightingToken[];
+  // Whether the line is part of an inactive preprocessor branch.
+  isInactive: boolean;
 }
 
 // Language server push notification providing the semantic highlighting
@@ -122,7 +126,10 @@
 
   handleNotification(params: SemanticHighlightingParams) {
 const lines: SemanticHighlightingLine[] = params.lines.map(
-(line) => ({line : line.line, tokens : decodeTokens(line.tokens)}));
+  (line) => ({
+line: line.line, tokens: decodeTokens(line.tokens),
+isInactive: line.isInactive || false
+  }));
 this.highlighter.highlight(vscode.Uri.parse(params.textDocument.uri),
lines);
   }
@@ -161,6 +168,7 @@
   // SemanticHighlightingToken with scopeIndex i should have the decoration at
   // index i in this list.
   private decorationTypes: vscode.TextEditorDecorationType[] = [];
+  private inactiveDecorationIndex: number;
   // The clangd TextMate scope lookup table.
   private scopeLookupTable: string[][];
   constructor(scopeLookupTable: string[][]) {
@@ -180,7 +188,22 @@
   // theme is loaded.
   public 

[PATCH] D67537: [clangd] Client-side support for inactive regions

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

> Do you mean naming the scope something like `clangd.meta.disable` to make it 
> clear it's a clangd extension?
> 
> (In that case, we might as well make it something more descriptive, like 
> `clangd.preprocessor.inactive`. `meta` doesn't convey much information.)

yes, this was my suggestion.




Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:193
+  if (scopes[0] == "meta.disabled") {
+this.inactiveDecorationIndex = index;
+return vscode.window.createTextEditorDecorationType({

hmm, looks like clangd 
[omits](https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clangd/SemanticHighlighting.cpp#L454)
 the `InactiveCode` token.

I'd suggest we send it like other tokens, that will simplify the patch, we 
don't need the `inactiveDecorationIndex` and `isInactive`, I think it also 
helps when we migrate to the official implementation. WDYT?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67537



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67537: [clangd] Client-side support for inactive regions

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon times-circle color=red} Unit tests: fail. 62144 tests passed, 5 failed 
and 811 were skipped.

  failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
  failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

{icon times-circle color=red} clang-tidy: fail. clang-tidy found 2 errors and 0 
warnings 
.
 0 of them are added as review comments below (why? 
).

{icon times-circle color=red} clang-format: fail. Please format your changes 
with clang-format by running `git-clang-format HEAD^` or applying this patch 
.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 


//Pre-merge checks is in beta. Report issue 
.
 Please join beta  or enable 
it for your project 
.//


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67537



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67537: [clangd] Client-side support for inactive regions

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 240026.
nridge marked an inline comment as done.
nridge added a comment.

Inject the inactive code decoration type into Highligher.decorationTypes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67537

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
  
clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts

Index: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
@@ -63,7 +63,8 @@
   test('Colorizer groups decorations correctly', async () => {
 const scopeTable = [
   [ 'variable' ], [ 'entity.type.function' ],
-  [ 'entity.type.function.method' ]
+  [ 'entity.type.function.method' ],
+  [ 'meta.disabled' ]
 ];
 // Create the scope source ranges the highlightings should be highlighted
 // at. Assumes the scopes used are the ones in the "scopeTable" variable.
@@ -80,6 +81,12 @@
   new vscode.Position(line.line,
   token.character + token.length)));
 });
+if (line.isInactive) {
+  scopeRanges[scopeRanges.length - 1].push(new vscode.Range(
+new vscode.Position(line.line, 0),
+new vscode.Position(line.line, 0)
+  ));
+}
   });
   return scopeRanges;
 };
@@ -121,7 +128,8 @@
 tokens : [
   {character : 1, length : 2, scopeIndex : 1},
   {character : 10, length : 2, scopeIndex : 2},
-]
+],
+isInactive: false
   },
   {
 line : 2,
@@ -129,7 +137,8 @@
   {character : 3, length : 2, scopeIndex : 1},
   {character : 6, length : 2, scopeIndex : 1},
   {character : 8, length : 2, scopeIndex : 2},
-]
+],
+isInactive: true
   },
 ];
 
@@ -144,7 +153,8 @@
   line : 1,
   tokens : [
 {character : 2, length : 1, scopeIndex : 0},
-  ]
+  ],
+  isInactive: false
 };
 highlighter.highlight(fileUri2, [ highlightingsInLine1 ]);
 assert.deepEqual(
Index: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
@@ -23,6 +23,8 @@
   // with its start position, length and the "lookup table" index of of the
   // semantic highlighting Text Mate scopes.
   tokens?: string;
+  // Whether the line is part of an inactive preprocessor branch.
+  isInactive?: boolean;
 }
 
 // A SemanticHighlightingToken decoded from the base64 data sent by clangd.
@@ -40,6 +42,8 @@
   line: number;
   // All SemanticHighlightingTokens on the line.
   tokens: SemanticHighlightingToken[];
+  // Whether the line is part of an inactive preprocessor branch.
+  isInactive: boolean;
 }
 
 // Language server push notification providing the semantic highlighting
@@ -122,7 +126,10 @@
 
   handleNotification(params: SemanticHighlightingParams) {
 const lines: SemanticHighlightingLine[] = params.lines.map(
-(line) => ({line : line.line, tokens : decodeTokens(line.tokens)}));
+  (line) => ({
+line: line.line, tokens: decodeTokens(line.tokens),
+isInactive: line.isInactive || false
+  }));
 this.highlighter.highlight(vscode.Uri.parse(params.textDocument.uri),
lines);
   }
@@ -161,6 +168,7 @@
   // SemanticHighlightingToken with scopeIndex i should have the decoration at
   // index i in this list.
   private decorationTypes: vscode.TextEditorDecorationType[] = [];
+  private inactiveDecorationIndex: number;
   // The clangd TextMate scope lookup table.
   private scopeLookupTable: string[][];
   constructor(scopeLookupTable: string[][]) {
@@ -180,7 +188,22 @@
   // theme is loaded.
   public initialize(themeRuleMatcher: ThemeRuleMatcher) {
 this.decorationTypes.forEach((t) => t.dispose());
-this.decorationTypes = this.scopeLookupTable.map((scopes) => {
+this.decorationTypes = this.scopeLookupTable.map((scopes, index) => {
+  if (scopes[0] == "meta.disabled") {
+this.inactiveDecorationIndex = index;
+return vscode.window.createTextEditorDecorationType({
+  isWholeLine: true,
+  // FIXME: Avoid hardcoding these colors.
+  light: {
+color: "rgb(100, 100, 100)",
+backgroundColor: "rgba(220, 220, 220, 0.3)"
+  },
+  dark: {
+color: "rgb(100, 100, 

[PATCH] D67537: [clangd] Client-side support for inactive regions

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D67537#1833987 , @hokein wrote:

> there is some significant progress about semantic highlighting on VSCode,  
> VSCode now provides an experimental semantic token API 
> , 
> but no LSP binding yet (is not far away I think). When it is ready, we'd like 
> to switch to that and get rid of our own implementation.


Agreed.

(One potential issue is that vscode themes don't support background styling 
. However, that issue has been 
getting a lot of attention, hopefully it too will be fixed soon.)

I do think there's value in landing this patch in the interim.

> Looking at the API there, it seems like we could make the line-style scope 
> name `meta.disable` clangd specific, and define our own style.

Do you mean naming the scope something like `clangd.meta.disable` to make it 
clear it's a clangd extension?

(In that case, we might as well make it something more descriptive, like 
`clangd.preprocessor.inactive`. `meta` doesn't convey much information.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67537



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67537: [clangd] Client-side support for inactive regions

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

sorry for the looong delay, I was OOO for a few weeks before.

The patch looks fine, my concern is that how do we support this `line` 
extension when LSP provides a standard implementation -- there is some 
significant progress about semantic highlighting on VSCode,  VSCode now 
provides an experimental semantic token API 
, but 
no LSP binding yet (is not far away I think). When it is ready, we'd like to 
switch to that and get rid of our own implementation. Looking at the API there, 
it seems like we could make the line-style scope name `meta.disable` clangd 
specific, and define our own style.




Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:293
   });
+  if (line.isInactive) {
+inactiveRanges.push(new vscode.Range(

 another solution that requires minimal changes (but more hacky) is that, since 
clangd emits a `meta.disabled` token with line-set-only range, we could inject 
the `inactiveCodeDecorationType` to the `meta.disabled` slot of 
`Highlighter::decorationTypes`, and everything should work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67537



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67537: [clangd] Client-side support for inactive regions

[Nathan Ridge via Phabricator via cfe-commits]

nridge added reviewers: kadircet, ilya-biryukov, sammccall.
nridge added a comment.

Review ping again :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67537



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67537: [clangd] Client-side support for inactive regions

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Review ping :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67537



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67537: [clangd] Client-side support for inactive regions

[Nathan Ridge via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:210
+  isWholeLine: true,
+  // FIXME: Avoid hardcoding these colors.
+  light: {

I'm open to suggestions for how we could avoid harcoding these colors. VSCode 
themes do not seem to include colors for `"meta.disabled"` or any similar scope 
that would fit this role.

Perhaps we can allow the user to define the colors in clangd-specifier setting 
names, e.g. `clangd.inactiveHighlightColor.light`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67537



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67537: [clangd] Client-side support for inactive regions

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 230586.
nridge added a comment.

Clean up patch a bit and update tests as well


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67537

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
  
clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts

Index: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
@@ -72,16 +72,25 @@
  semanticHighlighting.SemanticHighlightingLine[]) => {
   // Initialize the scope ranges list to the correct size. Otherwise
   // scopes that don't have any highlightings are missed.
-  let scopeRanges: vscode.Range[][] = scopeTable.map(() => []);
+  let tokenRanges: vscode.Range[][] = scopeTable.map(() => []);
+  let inactiveRanges: vscode.Range[] = [];
   highlightingLines.forEach((line) => {
 line.tokens.forEach((token) => {
-  scopeRanges[token.scopeIndex].push(new vscode.Range(
+  tokenRanges[token.scopeIndex].push(new vscode.Range(
   new vscode.Position(line.line, token.character),
   new vscode.Position(line.line,
   token.character + token.length)));
 });
+if (line.isInactive) {
+  inactiveRanges.push(new vscode.Range(
+new vscode.Position(line.line, 0),
+new vscode.Position(line.line, 0)
+  ));
+}
   });
-  return scopeRanges;
+  let result : semanticHighlighting.DecorationRanges = 
+{ tokenRanges, inactiveRanges };
+  return result;
 };
 
 const fileUri1 = vscode.Uri.parse('file:///file1');
@@ -121,7 +130,8 @@
 tokens : [
   {character : 1, length : 2, scopeIndex : 1},
   {character : 10, length : 2, scopeIndex : 2},
-]
+],
+isInactive: false
   },
   {
 line : 2,
@@ -129,7 +139,8 @@
   {character : 3, length : 2, scopeIndex : 1},
   {character : 6, length : 2, scopeIndex : 1},
   {character : 8, length : 2, scopeIndex : 2},
-]
+],
+isInactive: true
   },
 ];
 
@@ -144,7 +155,8 @@
   line : 1,
   tokens : [
 {character : 2, length : 1, scopeIndex : 0},
-  ]
+  ],
+  isInactive: false
 };
 highlighter.highlight(fileUri2, [ highlightingsInLine1 ]);
 assert.deepEqual(
@@ -167,7 +179,8 @@
 // Closing a text document removes all highlightings for the file and no
 // other files.
 highlighter.removeFileHighlightings(fileUri1);
-assert.deepEqual(highlighter.getDecorationRanges(fileUri1), []);
+assert.deepEqual(highlighter.getDecorationRanges(fileUri1),
+ { tokenRanges: [], inactiveRanges: [] });
 assert.deepEqual(highlighter.getDecorationRanges(fileUri2),
  createHighlightingScopeRanges([ highlightingsInLine1 ]));
   });
Index: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
===
--- clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
@@ -23,6 +23,8 @@
   // with its start position, length and the "lookup table" index of of the
   // semantic highlighting Text Mate scopes.
   tokens?: string;
+  // Whether the line is part of an inactive preprocessor branch.
+  isInactive?: boolean;
 }
 
 // A SemanticHighlightingToken decoded from the base64 data sent by clangd.
@@ -40,6 +42,13 @@
   line: number;
   // All SemanticHighlightingTokens on the line.
   tokens: SemanticHighlightingToken[];
+  // Whether the line is part of an inactive preprocessor branch.
+  isInactive: boolean;
+}
+
+export interface DecorationRanges {
+  tokenRanges: vscode.Range[][],
+  inactiveRanges: vscode.Range[]
 }
 
 // Language server push notification providing the semantic highlighting
@@ -122,7 +131,10 @@
 
   handleNotification(params: SemanticHighlightingParams) {
 const lines: SemanticHighlightingLine[] = params.lines.map(
-(line) => ({line : line.line, tokens : decodeTokens(line.tokens)}));
+  (line) => ({
+line: line.line, tokens: decodeTokens(line.tokens),
+isInactive: line.isInactive || false
+  }));
 this.highlighter.highlight(vscode.Uri.parse(params.textDocument.uri),
lines);
   }
@@ -161,6 +173,7 @@
   // SemanticHighlightingToken with scopeIndex i should have the