[PATCH] D66990: [clangd] Add distinct highlightings for declarations of functions and methods

[Nathan Ridge via Phabricator via cfe-commits]

nridge abandoned this revision.
nridge added a comment.

Abandoning as this was superseded by D77811 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66990

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

[PATCH] D66990: [clangd] Add distinct highlightings for declarations of functions and methods

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.
Herald added a subscriber: usaxena95.

In D66990#1656230 , @nridge wrote:

> >> I think the way cquery does it 
> >> 
> >>  it better in this regard: in place of a single kind enum, they 
> >> essentially have a 4-tuple of `(kind, parent kind, storage class, role)`.
> > 
> > A design like this definitely makes more sense. I was thinking of a 
> > slightly simpler model: adding a set of "modifiers" to each highlighting 
> > should be enough to encode all of it, e.g. a modifier 'is class member' 
> > could be used to distinguish methods and fields from global functions and 
> > variables, a modifier 'is usage' can be used to distinguish usages from 
> > declarations, etc.
> >  But there's no combinatorial explosion in the cquery's model, which is the 
> > important bit.
>
> I will suggest this for the upstream protocol and see where that goes.


For reference, I made this suggestion here 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66990



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

[PATCH] D66990: [clangd] Add distinct highlightings for declarations of functions and methods

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D66990#1656230 , @nridge wrote:

> Not a hard requirement, just a nice-to-have for someone moving from one tool 
> to another :)
>  If you feel that for now it's better not to do this, I can respect that.


Thanks, if that works for you, I would wait until we get/build a better support 
in the protocol for this.
If Eclipse CDT gets a lot of user complaints and this turns out to be an 
important missing feature, happy to reconsider. 
FWIW, I find having Eclipse CDT on board to be important for us and therefore 
happy to make trade-offs when needed.
This is a small thing, though, so waiting for an explicit support for this in 
the protocol does not sound too bad.

> I will suggest this for the upstream protocol and see where that goes.

SG, keep us posted!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66990



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

[PATCH] D66990: [clangd] Add distinct highlightings for declarations of functions and methods

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D66990#1655175 , @ilya-biryukov 
wrote:

> In D66990#1655030 , @nridge wrote:
>
> > I was hoping though that a patch like this, which would bring us largely to 
> > parity with Eclipse CDT's highlightings, wouldn't need to blocked on a 
> > change to the upstream protocol proposal, which could take a while.
>
>
> Is feature parity a hard requirement? Having slightly different highlightings 
> in that case should not be too disruptive.


Not a hard requirement, just a nice-to-have for someone moving from one tool to 
another :)

If you feel that for now it's better not to do this, I can respect that.

>> I think the way cquery does it 
>> 
>>  it better in this regard: in place of a single kind enum, they essentially 
>> have a 4-tuple of `(kind, parent kind, storage class, role)`.
> 
> A design like this definitely makes more sense. I was thinking of a slightly 
> simpler model: adding a set of "modifiers" to each highlighting should be 
> enough to encode all of it, e.g. a modifier 'is class member' could be used 
> to distinguish methods and fields from global functions and variables, a 
> modifier 'is usage' can be used to distinguish usages from declarations, etc.
>  But there's no combinatorial explosion in the cquery's model, which is the 
> important bit.

I will suggest this for the upstream protocol and see where that goes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66990



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

[PATCH] D66990: [clangd] Add distinct highlightings for declarations of functions and methods

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D66990#1655030 , @nridge wrote:

> I was hoping though that a patch like this, which would bring us largely to 
> parity with Eclipse CDT's highlightings, wouldn't need to blocked on a change 
> to the upstream protocol proposal, which could take a while.


Is feature parity a hard requirement? Having slightly different highlightings 
in that case should not be too disruptive.

> I think the way cquery does it 
> 
>  it better in this regard: in place of a single kind enum, they essentially 
> have a 4-tuple of `(kind, parent kind, storage class, role)`.

A design like this definitely makes more sense. I was thinking of a slightly 
simpler model: adding a set of "modifiers" to each highlighting should be 
enough to encode all of it, e.g. a modifier 'is class member' could be used to 
distinguish methods and fields from global functions and variables, a modifier 
'is usage' can be used to distinguish usages from declarations, etc.
But there's no combinatorial explosion in the cquery's model, which is the 
important bit.

>> That would also mean we would rely on every editor to apply consistent 
>> styling for this, at least by default: make `somethind-decl` just like 
>> `decl` but bold.
>>  I don't think there's any way to enforce this.

Different designs lead to different interpretations: if we encode this as a 
separate highlighting kind, there's a greater chance this would be highlighted 
in different color rather than just applying modifiers.
If we model this as "something in addition to the main highlighting", there's a 
greater chance this would be interpreted as something that should be "bold".

Agree there is no way to enforce this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66990



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

[PATCH] D66990: [clangd] Add distinct highlightings for declarations of functions and methods

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D66990#1654312 , @ilya-biryukov 
wrote:

> I think this makes sense, but it should be done for **all** declarations and 
> not just functions.
>  But doing that in the current design would mean we need to double the number 
> of styles. In addition to `field`, `method`, `function`, we would need 
> `field-declaration`, `method-declaration`, `function-declaration`, etc.
>  Combinatorial explosion of that kind is bad, so I would rather avoid it.


I agree that a combinatorial explosion would be unfortunate. I actually think 
that having a linear list of semantic highlighting kinds is a weakness of this 
protocol extension for exactly this reason.

I think the way cquery does it 

 it better in this regard: in place of a single kind enum, they essentially 
have a 4-tuple of `(kind, parent kind, storage class, role)`. With this setup:

- The kinds can be a relatively small set of fundamental language entity kinds 
(variable, function, class, namespace, etc.)
- Things like "field" can be expressed as `kind = variable, parentKind = class`
- The storage class allows differentiating things like static vs. nonstatic 
without having to introduce distinct kinds for those.
- The role allows differentiating declarations from uses, and even fancier 
things like read occurrences from write occurrences.

I've been meaning to comment on the upstream protocol proposal to suggest a 
more flexible approach like this, though I'm not sure how it would integrate 
with TextMate scopes (which cquery doesn't bother with).

I was hoping though that a patch like this, which would bring us largely to 
parity with Eclipse CDT's highlightings, wouldn't need to blocked on a change 
to the upstream protocol proposal, which could take a while.

> That would also mean we would rely on every editor to apply consistent 
> styling for this, at least by default: make `somethind-decl` just like `decl` 
> but bold.
>  I don't think there's any way to enforce this.

Keep in mind that since the scopes for declarations just add `.declaration` to 
the scopes for the base entities, by default they would be highlighted the same 
as the base entity. Only if a user or theme goes to the trouble of adding a 
specific rule for the `.declaration`, would they look different, and in that 
case I think we can trust the judgment of the user or theme author.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66990



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

[PATCH] D66990: [clangd] Add distinct highlightings for declarations of functions and methods

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D66990#1653833 , @nridge wrote:

> I think it makes sense to style function declarations differently from 
> function uses for emphasis. For example, you can use the same color for both, 
> but make the declarations bold. I have found this to aid readability.


I think this makes sense, but it should be done for **all** declarations and 
not just functions.
But doing that in the current design would mean we need to double the number of 
styles. In addition to `field`, `method`, `function`, we would need 
`field-declaration`, `method-declaration`, `function-declaration`, etc.
Combinatorial explosion of that kind is bad, so I would rather avoid it.

That would also mean we would rely on every editor to apply consistent styling 
for this, at least by default: make `somethind-decl` just like `decl` but bold.
I don't think there's any way to enforce this.

If there was an alternative design we could use for this that would not cause 
combinatorial explosion and would allow us to consistently apply the same 
modifier to different kinds of highlighting (e.g. something that will 
**consistently** turn to bold in all editors that support this), I would be 
supportive of implementing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66990



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

[PATCH] D66990: [clangd] Add distinct highlightings for declarations of functions and methods

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D66990#1653833 , @nridge wrote:

> There is precedent for highlighting declarations and uses differently in 
> other C++ editors. For example, Eclipse CDT has separate highlightings for 
> function and functions declarations (see screenshot below).


Acknowledged.

> Objectively speaking, I think it makes sense to style function declarations 
> differently from function uses for emphasis. For example, you can use the 
> same color for both, but make the declarations bold. I have found this to aid 
> readability.

As mentioned before, I think doing this:

- adds some confusion: the **same** thing highlighted in **two** different 
colors,
- provides almost zero value: function declarations have completely different 
syntax from function usages anyway, there are more than enough signals to 
distinguish them and we don't need semantic highlighting for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66990



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

[PATCH] D66990: [clangd] Add distinct highlightings for declarations of functions and methods

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

There is precedent for highlighting declarations and uses differently in other 
C++ editors. For example, Eclipse CDT has separate highlightings for function 
and functions declarations (see screenshot below).

Objectively speaking, I think it makes sense to style function declarations 
differently from function uses for emphasis. For example, you can use the same 
color for both, but make the declarations bold. I have found this to aid 
readability.

F9891365: cdt-syntax-color.png 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66990



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

[PATCH] D66990: [clangd] Add distinct highlightings for declarations of functions and methods

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D66990#1652172 , @jvikstrom wrote:

> Should we have different highlightings for declarations vs usages? (Although 
> I guess in the end it will be up to the editor if they highlight them 
> differently as the scope is just more specific for declarations)
>  I guess I personally don't really see the reason as it should be clear from 
> the context if it's a declaration or a function call.


I also second the opinion that usages and declarations should be highlighted 
the same.
Both represent the **same** thing, therefore they should have the **same** 
color.

And this highlighting does not seem to add any extra information too - it's 
obvious from the code whether a name is a declaration or a usage.
Even in the macro cases, the users usually know the semantics of the macro or 
can infer from the name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66990



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

[PATCH] D66990: [clangd] Add distinct highlightings for declarations of functions and methods

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom added a comment.

Should we have different highlightings for declarations vs usages? (Although I 
guess in the end it will be up to the editor if they highlight them differently 
as the scope is just more specific for declarations)
I guess I personally don't really see the reason as it should be clear from the 
context if it's a declaration or a function call.
But this might actually be nice if you have macros that declare functions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66990



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

[PATCH] D66990: [clangd] Add distinct highlightings for declarations of functions and methods

[Nathan Ridge via Phabricator via cfe-commits]

nridge created this revision.
nridge added reviewers: hokein, ilya-biryukov, jvikstrom.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66990

Files:
  clang-tools-extra/clangd/SemanticHighlighting.cpp
  clang-tools-extra/clangd/SemanticHighlighting.h
  clang-tools-extra/clangd/test/semantic-highlighting.test
  clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -37,7 +37,8 @@
   EXPECT_EQ(apply("^if (true) {return 100;} else {continue;}"),
 "if (true) {continue;} else {return 100;}");
   EXPECT_EQ(apply("^if () {return 100;} else {continue;}"),
-"if () {continue;} else {return 100;}") << "broken condition";
+"if () {continue;} else {return 100;}")
+  << "broken condition";
   EXPECT_AVAILABLE("^i^f^^(^t^r^u^e^) { return 100; } ^e^l^s^e^ { continue; }");
   EXPECT_UNAVAILABLE("if (true) {^return ^100;^ } else { ^continue^;^ }");
   // Available in subexpressions of the condition;
@@ -68,7 +69,7 @@
   EXPECT_UNAVAILABLE(R"cpp(R"(multi )" ^"token " u8"str\ning")cpp"); // nonascii
   EXPECT_UNAVAILABLE(R"cpp(^R^"^(^multi )" "token " "str\ning")cpp"); // raw
   EXPECT_UNAVAILABLE(R"cpp(^"token\n" __FILE__)cpp"); // chunk is macro
-  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp"); // forbidden escape char
+  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp");   // forbidden escape char
 
   const char *Input = R"cpp(R"(multi
 token)" "\nst^ring\n" "literal")cpp";
@@ -254,11 +255,11 @@
  void f(int a) {
int y = PLUS([[1+a]]);
  })cpp",
-  /*FIXME: It should be extracted like this.
-   R"cpp(#define PLUS(x) x++
- void f(int a) {
-   auto dummy = 1+a; int y = PLUS(dummy);
- })cpp"},*/
+   /*FIXME: It should be extracted like this.
+R"cpp(#define PLUS(x) x++
+  void f(int a) {
+auto dummy = 1+a; int y = PLUS(dummy);
+  })cpp"},*/
R"cpp(#define PLUS(x) x++
  void f(int a) {
auto dummy = PLUS(1+a); int y = dummy;
@@ -269,13 +270,13 @@
if(1)
 LOOP(5 + [[3]])
  })cpp",
-  /*FIXME: It should be extracted like this. SelectionTree needs to be
-* fixed for macros.
-   R"cpp(#define LOOP(x) while (1) {a = x;}
-   void f(int a) {
- auto dummy = 3; if(1)
-  LOOP(5 + dummy)
-   })cpp"},*/
+   /*FIXME: It should be extracted like this. SelectionTree needs to be
+ * fixed for macros.
+R"cpp(#define LOOP(x) while (1) {a = x;}
+void f(int a) {
+  auto dummy = 3; if(1)
+   LOOP(5 + dummy)
+})cpp"},*/
R"cpp(#define LOOP(x) while (1) {a = x;}
  void f(int a) {
auto dummy = LOOP(5 + 3); if(1)
@@ -371,8 +372,8 @@
  void f() {
auto dummy = S(2) + S(3) + S(4); S x = S(1) + dummy + S(5);
  })cpp"},
-   // Don't try to analyze across macro boundaries
-   // FIXME: it'd be nice to do this someday (in a safe way)
+  // Don't try to analyze across macro boundaries
+  // FIXME: it'd be nice to do this someday (in a safe way)
   {R"cpp(#define ECHO(X) X
  void f() {
int x = 1 + [[ECHO(2 + 3) + 4]] + 5;
@@ -400,19 +401,19 @@
   EXPECT_AVAILABLE("^vo^id^ ^f(^) {^}^"); // available everywhere.
   EXPECT_AVAILABLE("[[int a; int b;]]");
   EXPECT_EQ("/* storage.type.primitive.cpp */void "
-"/* entity.name.function.cpp */f() {}",
+"/* entity.name.function.declaration.cpp */f() {}",
 apply("void ^f() {}"));
 
   EXPECT_EQ(apply("[[void f1(); void f2();]]"),
 "/* storage.type.primitive.cpp */void "
-"/* entity.name.function.cpp */f1(); "
+"/* entity.name.function.declaration.cpp */f1(); "
 "/* storage.type.primitive.cpp */void "
-"/* entity.name.function.cpp */f2();");
+"/* entity.name.function.declaration.cpp */f2();");
 
   EXPECT_EQ(apply("void f1(); void f2() {^}"),
 "void f1(); "
 "/* storage.type.primitive.cpp */void "
-"/* entity.name.function.cpp */f2() {}");
+"/* entity.name.function.declaration.cpp */f2() {}");
 }
 
 TWEAK_TEST(ExpandMacro);
@@ -489,7 +490,7 @@
   StartsWith("fail: Could