[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Nathan Ridge via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG37e31e629dc1: [clangd] Improve semantic highlighting in 
dependent contexts (fixes #154) (authored by nridge).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901

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

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -51,6 +51,9 @@
   {HighlightingKind::StaticField, "StaticField"},
   {HighlightingKind::Method, "Method"},
   {HighlightingKind::StaticMethod, "StaticMethod"},
+  {HighlightingKind::Typedef, "Typedef"},
+  {HighlightingKind::DependentType, "DependentType"},
+  {HighlightingKind::DependentName, "DependentName"},
   {HighlightingKind::TemplateParameter, "TemplateParameter"},
   {HighlightingKind::Primitive, "Primitive"},
   {HighlightingKind::Macro, "Macro"}};
@@ -181,7 +184,7 @@
   }
   template
   struct $Class[[C]] : $Namespace[[abc]]::$Class[[A]]<$TemplateParameter[[T]]> {
-typename $TemplateParameter[[T]]::A* $Field[[D]];
+typename $TemplateParameter[[T]]::$DependentType[[A]]* $Field[[D]];
   };
   $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Variable[[AA]];
   typedef $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Class[[AAA]];
@@ -529,6 +532,58 @@
   $Typedef[[Pointer]], $Typedef[[LVReference]], $Typedef[[RVReference]],
   $Typedef[[Array]], $Typedef[[MemberPointer]]);
   };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[phase1]]($TemplateParameter[[T]]);
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$Function[[phase1]]($Parameter[[P]]);
+$DependentName[[phase2]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  class $Class[[A]] {
+template 
+$Primitive[[void]] $Method[[bar]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[U]] $Parameter[[P]]) {
+$Class[[A]]().$Method[[bar]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  struct $Class[[A]] {
+template 
+static $Primitive[[void]] $StaticMethod[[foo]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  struct $Class[[B]] {
+$Primitive[[void]] $Method[[bar]]() {
+  $Class[[A]]::$StaticMethod[[foo]]($TemplateParameter[[T]]());
+}
+  };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]](typename $TemplateParameter[[T]]::$DependentType[[Type]]
+= $TemplateParameter[[T]]::$DependentName[[val]]);
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$Parameter[[P]].$DependentName[[Field]];
+  }
+)cpp",
+  R"cpp(
+  template 
+  class $Class[[A]] {
+$Primitive[[int]] $Method[[foo]]() {
+  return $TemplateParameter[[T]]::$DependentName[[Field]];
+}
+  };
 )cpp"};
   for (const auto  : TestCases) {
 checkHighlightings(TestCase);
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
@@ -41,6 +41,12 @@
 # CHECK-NEXT:"entity.name.type.typedef.cpp"
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
+# CHECK-NEXT:"entity.name.type.dependent.cpp"
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  [
+# CHECK-NEXT:"entity.name.other.dependent.cpp"
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  [
 # CHECK-NEXT:"entity.name.namespace.cpp"
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
@@ -61,7 +67,7 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:],
 # CHECK-NEXT:"textDocument": {
@@ -76,11 +82,11 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 1,
-# 

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Nathan Ridge via Phabricator via cfe-commits]

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

Address last review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901

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

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -51,6 +51,9 @@
   {HighlightingKind::StaticField, "StaticField"},
   {HighlightingKind::Method, "Method"},
   {HighlightingKind::StaticMethod, "StaticMethod"},
+  {HighlightingKind::Typedef, "Typedef"},
+  {HighlightingKind::DependentType, "DependentType"},
+  {HighlightingKind::DependentName, "DependentName"},
   {HighlightingKind::TemplateParameter, "TemplateParameter"},
   {HighlightingKind::Primitive, "Primitive"},
   {HighlightingKind::Macro, "Macro"}};
@@ -175,7 +178,7 @@
   }
   template
   struct $Class[[C]] : $Namespace[[abc]]::$Class[[A]]<$TemplateParameter[[T]]> {
-typename $TemplateParameter[[T]]::A* $Field[[D]];
+typename $TemplateParameter[[T]]::$DependentType[[A]]* $Field[[D]];
   };
   $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Variable[[AA]];
   typedef $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Class[[AAA]];
@@ -523,6 +526,58 @@
   $Typedef[[Pointer]], $Typedef[[LVReference]], $Typedef[[RVReference]],
   $Typedef[[Array]], $Typedef[[MemberPointer]]);
   };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[phase1]]($TemplateParameter[[T]]);
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$Function[[phase1]]($Parameter[[P]]);
+$DependentName[[phase2]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  class $Class[[A]] {
+template 
+$Primitive[[void]] $Method[[bar]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[U]] $Parameter[[P]]) {
+$Class[[A]]().$Method[[bar]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  struct $Class[[A]] {
+template 
+static $Primitive[[void]] $StaticMethod[[foo]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  struct $Class[[B]] {
+$Primitive[[void]] $Method[[bar]]() {
+  $Class[[A]]::$StaticMethod[[foo]]($TemplateParameter[[T]]());
+}
+  };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]](typename $TemplateParameter[[T]]::$DependentType[[Type]]
+= $TemplateParameter[[T]]::$DependentName[[val]]);
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$Parameter[[P]].$DependentName[[Field]];
+  }
+)cpp",
+  R"cpp(
+  template 
+  class $Class[[A]] {
+$Primitive[[int]] $Method[[foo]]() {
+  return $TemplateParameter[[T]]::$DependentName[[Field]];
+}
+  };
 )cpp"};
   for (const auto  : TestCases) {
 checkHighlightings(TestCase);
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
@@ -41,6 +41,12 @@
 # CHECK-NEXT:"entity.name.type.typedef.cpp"
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
+# CHECK-NEXT:"entity.name.type.dependent.cpp"
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  [
+# CHECK-NEXT:"entity.name.other.dependent.cpp"
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  [
 # CHECK-NEXT:"entity.name.namespace.cpp"
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
@@ -61,7 +67,7 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:],
 # CHECK-NEXT:"textDocument": {
@@ -76,11 +82,11 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 1,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# 

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[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, looks great now!




Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:41
+  if (auto *TD = dyn_cast(D)) {
+if (auto *Templated = TD->getTemplatedDecl()) {
+  D = Templated;

nit: remove the enclosing `{}`.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:107
+llvm::Optional kindForCandidateDecls(
+llvm::iterator_range Decls) {
+  llvm::Optional Result;

nit: the parameter type seems a bit tangled, may use 
`llvm::iterator_range`, but it is not a big deal, up to 
you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

LGTM from my side, but not marking as accepted yet to make sure @hokein has a 
chance to take a final look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Nathan Ridge via Phabricator via cfe-commits]

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

Address final comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901

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

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -51,6 +51,9 @@
   {HighlightingKind::StaticField, "StaticField"},
   {HighlightingKind::Method, "Method"},
   {HighlightingKind::StaticMethod, "StaticMethod"},
+  {HighlightingKind::Typedef, "Typedef"},
+  {HighlightingKind::DependentType, "DependentType"},
+  {HighlightingKind::DependentName, "DependentName"},
   {HighlightingKind::TemplateParameter, "TemplateParameter"},
   {HighlightingKind::Primitive, "Primitive"},
   {HighlightingKind::Macro, "Macro"}};
@@ -175,7 +178,7 @@
   }
   template
   struct $Class[[C]] : $Namespace[[abc]]::$Class[[A]]<$TemplateParameter[[T]]> {
-typename $TemplateParameter[[T]]::A* $Field[[D]];
+typename $TemplateParameter[[T]]::$DependentType[[A]]* $Field[[D]];
   };
   $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Variable[[AA]];
   typedef $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Class[[AAA]];
@@ -523,6 +526,58 @@
   $Typedef[[Pointer]], $Typedef[[LVReference]], $Typedef[[RVReference]],
   $Typedef[[Array]], $Typedef[[MemberPointer]]);
   };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[phase1]]($TemplateParameter[[T]]);
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$Function[[phase1]]($Parameter[[P]]);
+$DependentName[[phase2]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  class $Class[[A]] {
+template 
+$Primitive[[void]] $Method[[bar]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[U]] $Parameter[[P]]) {
+$Class[[A]]().$Method[[bar]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  struct $Class[[A]] {
+template 
+static $Primitive[[void]] $StaticMethod[[foo]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  struct $Class[[B]] {
+$Primitive[[void]] $Method[[bar]]() {
+  $Class[[A]]::$StaticMethod[[foo]]($TemplateParameter[[T]]());
+}
+  };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]](typename $TemplateParameter[[T]]::$DependentType[[Type]]
+= $TemplateParameter[[T]]::$DependentName[[val]]);
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$Parameter[[P]].$DependentName[[Field]];
+  }
+)cpp",
+  R"cpp(
+  template 
+  class $Class[[A]] {
+$Primitive[[int]] $Method[[foo]]() {
+  return $TemplateParameter[[T]]::$DependentName[[Field]];
+}
+  };
 )cpp"};
   for (const auto  : TestCases) {
 checkHighlightings(TestCase);
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
@@ -41,6 +41,12 @@
 # CHECK-NEXT:"entity.name.type.typedef.cpp"
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
+# CHECK-NEXT:"entity.name.type.dependent.cpp"
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  [
+# CHECK-NEXT:"entity.name.other.dependent.cpp"
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  [
 # CHECK-NEXT:"entity.name.namespace.cpp"
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
@@ -61,7 +67,7 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:],
 # CHECK-NEXT:"textDocument": {
@@ -76,11 +82,11 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 1,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D67901#1704639 , @nridge wrote:

> I like how we went from using heuristics, to not using heuristics, to using 
> heuristics again :)
>
> But admittedly, the new heuristics are more accurate because they're based on 
> phase 1 lookup results rather than just syntax, so I'm happy with the outcome!


I also liked the irony of it :-)

Mostly LG, just NITs from my side. Happy to LGTM when they're addressed.




Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:105
+  for (NamedDecl *Decl : Decls) {
+if (TemplateDecl *TD = dyn_cast(Decl)) {
+  Decl = TD->getTemplatedDecl();

Could we do this in `kindForDecl` instead?
I suspect we want to be consistent in other cases this might potentially called 
in.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:108
+}
+llvm::Optional Kind = kindForDecl(Decl);
+if (!Kind)

Maybe simplify the rest of the loop body to the following code?
```
auto Kind = ...;
if (!Kind || Result && Result != Kind)
  return llvm::None;
Result = Kind;
```

If you want to have fewer assignments, we could also do:
```
if (!Result) Result = Kind;
```
at the end. But I'd just keep it a tad simpler instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 3 inline comments as done.
nridge added a comment.

I like how we went from using heuristics, to not using heuristics, to using 
heuristics again :)

But admittedly, the new heuristics are more accurate because they're based on 
phase 1 lookup results rather than just syntax, so I'm happy with the outcome!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Nathan Ridge via Phabricator via cfe-commits]

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

Updated to use heuristics based on OverloadExpr::decl()

Also folded VisitUnresolvedLookupExpr and VisitUnresolvedMemberExpr together
into a single VisitOverloadExpr


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901

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

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -51,6 +51,9 @@
   {HighlightingKind::StaticField, "StaticField"},
   {HighlightingKind::Method, "Method"},
   {HighlightingKind::StaticMethod, "StaticMethod"},
+  {HighlightingKind::Typedef, "Typedef"},
+  {HighlightingKind::DependentType, "DependentType"},
+  {HighlightingKind::DependentName, "DependentName"},
   {HighlightingKind::TemplateParameter, "TemplateParameter"},
   {HighlightingKind::Primitive, "Primitive"},
   {HighlightingKind::Macro, "Macro"}};
@@ -175,7 +178,7 @@
   }
   template
   struct $Class[[C]] : $Namespace[[abc]]::$Class[[A]]<$TemplateParameter[[T]]> {
-typename $TemplateParameter[[T]]::A* $Field[[D]];
+typename $TemplateParameter[[T]]::$DependentType[[A]]* $Field[[D]];
   };
   $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Variable[[AA]];
   typedef $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Class[[AAA]];
@@ -523,6 +526,58 @@
   $Typedef[[Pointer]], $Typedef[[LVReference]], $Typedef[[RVReference]],
   $Typedef[[Array]], $Typedef[[MemberPointer]]);
   };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[phase1]]($TemplateParameter[[T]]);
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$Function[[phase1]]($Parameter[[P]]);
+$DependentName[[phase2]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  class $Class[[A]] {
+template 
+$Primitive[[void]] $Method[[bar]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[U]] $Parameter[[P]]) {
+$Class[[A]]().$Method[[bar]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  struct $Class[[A]] {
+template 
+static $Primitive[[void]] $StaticMethod[[foo]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  struct $Class[[B]] {
+$Primitive[[void]] $Method[[bar]]() {
+  $Class[[A]]::$StaticMethod[[foo]]($TemplateParameter[[T]]());
+}
+  };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]](typename $TemplateParameter[[T]]::$DependentType[[Type]]
+= $TemplateParameter[[T]]::$DependentName[[val]]);
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$Parameter[[P]].$DependentName[[Field]];
+  }
+)cpp",
+  R"cpp(
+  template 
+  class $Class[[A]] {
+$Primitive[[int]] $Method[[foo]]() {
+  return $TemplateParameter[[T]]::$DependentName[[Field]];
+}
+  };
 )cpp"};
   for (const auto  : TestCases) {
 checkHighlightings(TestCase);
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
@@ -41,6 +41,12 @@
 # CHECK-NEXT:"entity.name.type.typedef.cpp"
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
+# CHECK-NEXT:"entity.name.type.dependent.cpp"
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  [
+# CHECK-NEXT:"entity.name.other.dependent.cpp"
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  [
 # CHECK-NEXT:"entity.name.namespace.cpp"
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
@@ -61,7 +67,7 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:],
 # CHECK-NEXT:"textDocument": {
@@ -76,11 +82,11 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:  {
 # 

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:157
+if (canHighlightName(E->getName()))
+  addToken(E->getNameLoc(), HighlightingKind::DependentName);
+return true;

Could we highlighting based on the kinds of `E->decls()`? If they're all the 
same, you could just use the corresponding highlighting kind.
Most of the time ADL at template instantiation time does not affect the 
resulting highlighting, so we could just assume it's going to stay the same.

If there are no `decls()` or some of them have different highlighting kinds 
(e.g. static and non-static member functions), you could fallback to 
`DependentName`.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:162
+  bool VisitUnresolvedMemberExpr(UnresolvedMemberExpr *E) {
+if (canHighlightName(E->getName()))
+  addToken(E->getNameLoc(), HighlightingKind::DependentName);

Same here. Could we try guessing the highlighting type based on `E->decls()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Nathan Ridge via Phabricator via cfe-commits]

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

Updated to use WalkUpFromDependentNameTypeLoc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901

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

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -51,6 +51,9 @@
   {HighlightingKind::StaticField, "StaticField"},
   {HighlightingKind::Method, "Method"},
   {HighlightingKind::StaticMethod, "StaticMethod"},
+  {HighlightingKind::Typedef, "Typedef"},
+  {HighlightingKind::DependentType, "DependentType"},
+  {HighlightingKind::DependentName, "DependentName"},
   {HighlightingKind::TemplateParameter, "TemplateParameter"},
   {HighlightingKind::Primitive, "Primitive"},
   {HighlightingKind::Macro, "Macro"}};
@@ -175,7 +178,7 @@
   }
   template
   struct $Class[[C]] : $Namespace[[abc]]::$Class[[A]]<$TemplateParameter[[T]]> {
-typename $TemplateParameter[[T]]::A* $Field[[D]];
+typename $TemplateParameter[[T]]::$DependentType[[A]]* $Field[[D]];
   };
   $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Variable[[AA]];
   typedef $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Class[[AAA]];
@@ -523,6 +526,55 @@
   $Typedef[[Pointer]], $Typedef[[LVReference]], $Typedef[[RVReference]],
   $Typedef[[Array]], $Typedef[[MemberPointer]]);
   };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$DependentName[[bar]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  class $Class[[A]] {
+template 
+$Primitive[[void]] $Method[[bar]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[U]] $Parameter[[P]]) {
+$Class[[A]]().$DependentName[[bar]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  struct $Class[[A]] {
+template 
+static $Primitive[[void]] $StaticMethod[[foo]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  struct $Class[[B]] {
+$Primitive[[void]] $Method[[bar]]() {
+  $Class[[A]]::$DependentName[[foo]]($TemplateParameter[[T]]());
+}
+  };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]](typename $TemplateParameter[[T]]::$DependentType[[Type]]
+= $TemplateParameter[[T]]::$DependentName[[val]]);
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$Parameter[[P]].$DependentName[[Field]];
+  }
+)cpp",
+  R"cpp(
+  template 
+  class $Class[[A]] {
+$Primitive[[int]] $Method[[foo]]() {
+  return $TemplateParameter[[T]]::$DependentName[[Field]];
+}
+  };
 )cpp"};
   for (const auto  : TestCases) {
 checkHighlightings(TestCase);
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
@@ -41,6 +41,12 @@
 # CHECK-NEXT:"entity.name.type.typedef.cpp"
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
+# CHECK-NEXT:"entity.name.type.dependent.cpp"
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  [
+# CHECK-NEXT:"entity.name.other.dependent.cpp"
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  [
 # CHECK-NEXT:"entity.name.namespace.cpp"
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
@@ -61,7 +67,7 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:],
 # CHECK-NEXT:"textDocument": {
@@ -76,11 +82,11 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 1,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:],
 # CHECK-NEXT:"textDocument": {
@@ -95,7 +101,7 @@
 # 

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Is this what you had in mind?

I'm not seeing where the `kindForType` part comes in. In particular, it seems 
like it would be silly to call `kindForType` in 
`WalkUpFromDependentNameTypeLoc()`, because we //know// the answer will be 
`HighlightingKind::DependentType`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:219
+  bool VisitDependentNameTypeLoc(DependentNameTypeLoc L) {
+addToken(L.getNameLoc(), HighlightingKind::DependentType);
+return true;

nridge wrote:
> ilya-biryukov wrote:
> > nridge wrote:
> > > hokein wrote:
> > > > nit: we have `kindForType` for hanlding all types, so I'd move the 
> > > > logic of detecting the dependent type there.
> > > I did try this, but it doesn't quite work, because `VisitTypeLoc` adds 
> > > the highlighting to the `TypeLoc`'s `getBeginLoc()`. For something like 
> > > `typename T::type`, that highlights the `typename` token rather than the 
> > > `type` token. By contrast, here I add the highlighting to the 
> > > `DependentNameTypeLoc`'s `getNameLoc()` which will correctly highlight 
> > > the `type` token.
> > You'd want to implement `WalkUpFromDependentNameTypeLoc` instead of 
> > `Visit*` to avoid adding extra highlightings in `VisitTypeLoc`.
> > 
> > In fact, I'm surprised we're not seeing them now.
> We're not seeing extra highlightings with the current patch, because 
> `VisitTypeLoc` does not add any highlightings for dependent types 
> (`kindForType()` returns `None` for them). 
> 
> So, I don't think there's a problem with using `VisitDependentNameTypeLoc`?
I'm second on Ilya's suggestion. It follows the existing pattern (see  
WalkupFrom* methods above) and make the code clearer, and we can move the 
`DependentType` to `kindForType`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 2 inline comments as done.
nridge added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:219
+  bool VisitDependentNameTypeLoc(DependentNameTypeLoc L) {
+addToken(L.getNameLoc(), HighlightingKind::DependentType);
+return true;

ilya-biryukov wrote:
> nridge wrote:
> > hokein wrote:
> > > nit: we have `kindForType` for hanlding all types, so I'd move the logic 
> > > of detecting the dependent type there.
> > I did try this, but it doesn't quite work, because `VisitTypeLoc` adds the 
> > highlighting to the `TypeLoc`'s `getBeginLoc()`. For something like 
> > `typename T::type`, that highlights the `typename` token rather than the 
> > `type` token. By contrast, here I add the highlighting to the 
> > `DependentNameTypeLoc`'s `getNameLoc()` which will correctly highlight the 
> > `type` token.
> You'd want to implement `WalkUpFromDependentNameTypeLoc` instead of `Visit*` 
> to avoid adding extra highlightings in `VisitTypeLoc`.
> 
> In fact, I'm surprised we're not seeing them now.
We're not seeing extra highlightings with the current patch, because 
`VisitTypeLoc` does not add any highlightings for dependent types 
(`kindForType()` returns `None` for them). 

So, I don't think there's a problem with using `VisitDependentNameTypeLoc`?



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:505
+  case HighlightingKind::DependentType:
+return "entity.name.type.dependent.cpp";
+  case HighlightingKind::DependentName:

hokein wrote:
> ilya-biryukov wrote:
> > Maybe have a separate category for all dependent entities instead, i.e. use
> > `entity.name.dependent.type.cpp` and `entity.name.dependent.other.cpp`?
> > 
> > This would allow to specify a single highlighting for both names by 
> > stopping at `dependent` subcategory.
> > I'm not sure how this plays into the default colors provided in the editors 
> > that support semantic highlighting...
> Having a dedicate dependent entity doesn't align with the existing textmate 
> patterns (the `dependent` type should be under the `entity.name.type` 
> umbrella) --  most highlighters don't have a specific pattern for 
> `dependent`, so we'll fallback to the `entity.name.type` color. If we use 
> `entity.name.dependent.type.cpp`, we'll fall back to `entity.name` color.
+1, I think it's more important that dependent types are highlighted as types 
out-of-the-box in cases where the theme contains a highlighting for types but 
not one specifically for dependent types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:505
+  case HighlightingKind::DependentType:
+return "entity.name.type.dependent.cpp";
+  case HighlightingKind::DependentName:

ilya-biryukov wrote:
> Maybe have a separate category for all dependent entities instead, i.e. use
> `entity.name.dependent.type.cpp` and `entity.name.dependent.other.cpp`?
> 
> This would allow to specify a single highlighting for both names by stopping 
> at `dependent` subcategory.
> I'm not sure how this plays into the default colors provided in the editors 
> that support semantic highlighting...
Having a dedicate dependent entity doesn't align with the existing textmate 
patterns (the `dependent` type should be under the `entity.name.type` umbrella) 
--  most highlighters don't have a specific pattern for `dependent`, so we'll 
fallback to the `entity.name.type` color. If we use 
`entity.name.dependent.type.cpp`, we'll fall back to `entity.name` color.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:505
+  case HighlightingKind::DependentType:
+return "entity.name.type.dependent.cpp";
+  case HighlightingKind::DependentName:

Maybe have a separate category for all dependent entities instead, i.e. use
`entity.name.dependent.type.cpp` and `entity.name.dependent.other.cpp`?

This would allow to specify a single highlighting for both names by stopping at 
`dependent` subcategory.
I'm not sure how this plays into the default colors provided in the editors 
that support semantic highlighting...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:219
+  bool VisitDependentNameTypeLoc(DependentNameTypeLoc L) {
+addToken(L.getNameLoc(), HighlightingKind::DependentType);
+return true;

nridge wrote:
> hokein wrote:
> > nit: we have `kindForType` for hanlding all types, so I'd move the logic of 
> > detecting the dependent type there.
> I did try this, but it doesn't quite work, because `VisitTypeLoc` adds the 
> highlighting to the `TypeLoc`'s `getBeginLoc()`. For something like `typename 
> T::type`, that highlights the `typename` token rather than the `type` token. 
> By contrast, here I add the highlighting to the `DependentNameTypeLoc`'s 
> `getNameLoc()` which will correctly highlight the `type` token.
You'd want to implement `WalkUpFromDependentNameTypeLoc` instead of `Visit*` to 
avoid adding extra highlightings in `VisitTypeLoc`.

In fact, I'm surprised we're not seeing them now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:219
+  bool VisitDependentNameTypeLoc(DependentNameTypeLoc L) {
+addToken(L.getNameLoc(), HighlightingKind::DependentType);
+return true;

hokein wrote:
> nit: we have `kindForType` for hanlding all types, so I'd move the logic of 
> detecting the dependent type there.
I did try this, but it doesn't quite work, because `VisitTypeLoc` adds the 
highlighting to the `TypeLoc`'s `getBeginLoc()`. For something like `typename 
T::type`, that highlights the `typename` token rather than the `type` token. By 
contrast, here I add the highlighting to the `DependentNameTypeLoc`'s 
`getNameLoc()` which will correctly highlight the `type` token.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 222725.
nridge marked 9 inline comments as done.
nridge added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901

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

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -51,6 +51,9 @@
   {HighlightingKind::StaticField, "StaticField"},
   {HighlightingKind::Method, "Method"},
   {HighlightingKind::StaticMethod, "StaticMethod"},
+  {HighlightingKind::Typedef, "Typedef"},
+  {HighlightingKind::DependentType, "DependentType"},
+  {HighlightingKind::DependentName, "DependentName"},
   {HighlightingKind::TemplateParameter, "TemplateParameter"},
   {HighlightingKind::Primitive, "Primitive"},
   {HighlightingKind::Macro, "Macro"}};
@@ -175,7 +178,7 @@
   }
   template
   struct $Class[[C]] : $Namespace[[abc]]::$Class[[A]]<$TemplateParameter[[T]]> {
-typename $TemplateParameter[[T]]::A* $Field[[D]];
+typename $TemplateParameter[[T]]::$DependentType[[A]]* $Field[[D]];
   };
   $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Variable[[AA]];
   typedef $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Class[[AAA]];
@@ -509,6 +512,55 @@
   $Typedef[[Pointer]], $Typedef[[LVReference]], $Typedef[[RVReference]],
   $Typedef[[Array]], $Typedef[[MemberPointer]]);
   };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$DependentName[[bar]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  class $Class[[A]] {
+template 
+$Primitive[[void]] $Method[[bar]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[U]] $Parameter[[P]]) {
+$Class[[A]]().$DependentName[[bar]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  struct $Class[[A]] {
+template 
+static $Primitive[[void]] $StaticMethod[[foo]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  struct $Class[[B]] {
+$Primitive[[void]] $Method[[bar]]() {
+  $Class[[A]]::$DependentName[[foo]]($TemplateParameter[[T]]());
+}
+  };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]](typename $TemplateParameter[[T]]::$DependentType[[Type]]
+= $TemplateParameter[[T]]::$DependentName[[val]]);
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$Parameter[[P]].$DependentName[[Field]];
+  }
+)cpp",
+  R"cpp(
+  template 
+  class $Class[[A]] {
+$Primitive[[int]] $Method[[foo]]() {
+  return $TemplateParameter[[T]]::$DependentName[[Field]];
+}
+  };
 )cpp"};
   for (const auto  : TestCases) {
 checkHighlightings(TestCase);
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
@@ -41,6 +41,12 @@
 # CHECK-NEXT:"entity.name.type.typedef.cpp"
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
+# CHECK-NEXT:"entity.name.type.dependent.cpp"
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  [
+# CHECK-NEXT:"entity.name.other.dependent.cpp"
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  [
 # CHECK-NEXT:"entity.name.namespace.cpp"
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
@@ -61,7 +67,7 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:],
 # CHECK-NEXT:"textDocument": {
@@ -76,11 +82,11 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 1,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:],
 # CHECK-NEXT:"textDocument": {
@@ -95,7 

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D67901#1687308 , @nridge wrote:

> I do feel strongly that types and non-types should be highlighted 
> differently, so the updated patch adds two new dependent highlightings, one 
> for types and one for variables/functions.


I see your point here, no objection.




Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:156
+  bool VisitUnresolvedLookupExpr(UnresolvedLookupExpr *E) {
+if (canHighlightName(E->getName())) {
+  addToken(E->getNameLoc(), HighlightingKind::DependentName);

nit: remove the `{}` and elsewhere, LLVM prefers not adding `{}` if the body 
only contains a single statement.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:163
+  bool VisitUnresolvedMemberExpr(UnresolvedMemberExpr *E) {
+if (canHighlightName(E->getMemberName())) {
+  addToken(E->getNameLoc(), HighlightingKind::DependentName);

I think we should use `E->getName()` since we are highlighting the `NameLoc` 
below.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:219
+  bool VisitDependentNameTypeLoc(DependentNameTypeLoc L) {
+addToken(L.getNameLoc(), HighlightingKind::DependentType);
+return true;

nit: we have `kindForType` for hanlding all types, so I'd move the logic of 
detecting the dependent type there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

I do feel strongly that types and non-types should be highlighted differently, 
so the updated patch adds two new dependent highlightings, one for types and 
one for variables/functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Nathan Ridge via Phabricator via cfe-commits]

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

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901

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

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -51,6 +51,9 @@
   {HighlightingKind::StaticField, "StaticField"},
   {HighlightingKind::Method, "Method"},
   {HighlightingKind::StaticMethod, "StaticMethod"},
+  {HighlightingKind::Typedef, "Typedef"},
+  {HighlightingKind::DependentType, "DependentType"},
+  {HighlightingKind::DependentName, "DependentName"},
   {HighlightingKind::TemplateParameter, "TemplateParameter"},
   {HighlightingKind::Primitive, "Primitive"},
   {HighlightingKind::Macro, "Macro"}};
@@ -175,7 +178,7 @@
   }
   template
   struct $Class[[C]] : $Namespace[[abc]]::$Class[[A]]<$TemplateParameter[[T]]> {
-typename $TemplateParameter[[T]]::A* $Field[[D]];
+typename $TemplateParameter[[T]]::$DependentType[[A]]* $Field[[D]];
   };
   $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Variable[[AA]];
   typedef $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Class[[AAA]];
@@ -509,6 +512,55 @@
   $Typedef[[Pointer]], $Typedef[[LVReference]], $Typedef[[RVReference]],
   $Typedef[[Array]], $Typedef[[MemberPointer]]);
   };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$DependentName[[bar]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  class $Class[[A]] {
+template 
+$Primitive[[void]] $Method[[bar]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[U]] $Parameter[[P]]) {
+$Class[[A]]().$DependentName[[bar]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  struct $Class[[A]] {
+template 
+static $Primitive[[void]] $StaticMethod[[foo]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  struct $Class[[B]] {
+$Primitive[[void]] $Method[[bar]]() {
+  $Class[[A]]::$DependentName[[foo]]($TemplateParameter[[T]]());
+}
+  };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]](typename $TemplateParameter[[T]]::$DependentType[[Type]]
+= $TemplateParameter[[T]]::$DependentName[[val]]);
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$Parameter[[P]].$DependentName[[Field]];
+  }
+)cpp",
+  R"cpp(
+  template 
+  class $Class[[A]] {
+$Primitive[[int]] $Method[[foo]]() {
+  return $TemplateParameter[[T]]::$DependentName[[Field]];
+}
+  };
 )cpp"};
   for (const auto  : TestCases) {
 checkHighlightings(TestCase);
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
@@ -41,6 +41,12 @@
 # CHECK-NEXT:"entity.name.type.typedef.cpp"
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
+# CHECK-NEXT:"entity.name.type.dependent.cpp"
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  [
+# CHECK-NEXT:"entity.name.other.dependent.cpp"
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  [
 # CHECK-NEXT:"entity.name.namespace.cpp"
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
@@ -61,7 +67,7 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:],
 # CHECK-NEXT:"textDocument": {
@@ -76,11 +82,11 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 1,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADABAEAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:],
 # CHECK-NEXT:"textDocument": {
@@ -95,7 +101,7 @@
 # CHECK-NEXT:"lines": [
 # 

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:181
+  addToken(E->getMemberLoc(), E->getQualifier()
+  ? HighlightingKind::StaticField
+  : HighlightingKind::Field);

nridge wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > nridge wrote:
> > > > hokein wrote:
> > > > > This could be member functions, a case is like
> > > > > 
> > > > > ```
> > > > > template
> > > > > class Foo {
> > > > > public:
> > > > >   void foo() {
> > > > > this->foo();
> > > > >   }
> > > > > };
> > > > > ```
> > > > Thanks for the example.
> > > > 
> > > > Do you have a suggestion for how to discriminate this case? To me, it 
> > > > would seem logical to do it based on syntax (highlight as a member 
> > > > function if the expression forms the function name of a function call 
> > > > expression). That would require navigating from the expression to its 
> > > > parent node. Is there a way to do that?
> > > There is no way to do this in C++.
> > > Even if the name is followed by a pair of parenthese, this could either 
> > > be a field with overloaded `operator()` (e.g. a `std::function 
> > > field`) or a function with the same name.
> > > 
> > > It's much better to pick a separate highlighting kind for dependent 
> > > names, this follows the actual semantics of C++.
> > +1, I think we should just highlight them as a dependent type.
> Of course, any attempt to disambiguate between a member function and a field 
> would be heuristic only. I figured that would be better than nothing. But if 
> you prefer using a separate highlighting for dependent names that resolve to 
> a function or a variable, we could do that.
> 
> (Hokein, I assume you don't actually mean using the dependent *type* 
> highlighting. Using a type highlighting for something we know is not a type, 
> but rather a function or variable, would be rather confusing.)
> But if you prefer using a separate highlighting for dependent names that 
> resolve to a function or a variable, we could do that.

I'd suggest we'd better follow this. The heuristic could fail in some cases.  
For these cases, the wrong highlighting'd confuse users too.

> (Hokein, I assume you don't actually mean using the dependent *type* 
> highlighting. Using a type highlighting for something we know is not a type, 
> but rather a function or variable, would be rather confusing.)

sorry for the confusion, I meant for anything that could not resolve to a 
specific declaration (e.g. `CXXDependentScopeMemberExpr`, 
`UnresolvedLookupExpr`), we use the new `dependent` highlighting kind.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 2 inline comments as done.
nridge added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:181
+  addToken(E->getMemberLoc(), E->getQualifier()
+  ? HighlightingKind::StaticField
+  : HighlightingKind::Field);

hokein wrote:
> ilya-biryukov wrote:
> > nridge wrote:
> > > hokein wrote:
> > > > This could be member functions, a case is like
> > > > 
> > > > ```
> > > > template
> > > > class Foo {
> > > > public:
> > > >   void foo() {
> > > > this->foo();
> > > >   }
> > > > };
> > > > ```
> > > Thanks for the example.
> > > 
> > > Do you have a suggestion for how to discriminate this case? To me, it 
> > > would seem logical to do it based on syntax (highlight as a member 
> > > function if the expression forms the function name of a function call 
> > > expression). That would require navigating from the expression to its 
> > > parent node. Is there a way to do that?
> > There is no way to do this in C++.
> > Even if the name is followed by a pair of parenthese, this could either be 
> > a field with overloaded `operator()` (e.g. a `std::function field`) 
> > or a function with the same name.
> > 
> > It's much better to pick a separate highlighting kind for dependent names, 
> > this follows the actual semantics of C++.
> +1, I think we should just highlight them as a dependent type.
Of course, any attempt to disambiguate between a member function and a field 
would be heuristic only. I figured that would be better than nothing. But if 
you prefer using a separate highlighting for dependent names that resolve to a 
function or a variable, we could do that.

(Hokein, I assume you don't actually mean using the dependent *type* 
highlighting. Using a type highlighting for something we know is not a type, 
but rather a function or variable, would be rather confusing.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:181
+  addToken(E->getMemberLoc(), E->getQualifier()
+  ? HighlightingKind::StaticField
+  : HighlightingKind::Field);

ilya-biryukov wrote:
> nridge wrote:
> > hokein wrote:
> > > This could be member functions, a case is like
> > > 
> > > ```
> > > template
> > > class Foo {
> > > public:
> > >   void foo() {
> > > this->foo();
> > >   }
> > > };
> > > ```
> > Thanks for the example.
> > 
> > Do you have a suggestion for how to discriminate this case? To me, it would 
> > seem logical to do it based on syntax (highlight as a member function if 
> > the expression forms the function name of a function call expression). That 
> > would require navigating from the expression to its parent node. Is there a 
> > way to do that?
> There is no way to do this in C++.
> Even if the name is followed by a pair of parenthese, this could either be a 
> field with overloaded `operator()` (e.g. a `std::function field`) or 
> a function with the same name.
> 
> It's much better to pick a separate highlighting kind for dependent names, 
> this follows the actual semantics of C++.
+1, I think we should just highlight them as a dependent type.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:159
+  addToken(E->getNameLoc(), E->getNamingClass()
+? HighlightingKind::StaticMethod
+: HighlightingKind::Function);

I'm doubting we will encounter other exceptions.

```
struct X {
template
static T t;
};

template
void f() {
  X::t; // this is a `UnresolvedLookupExpr`.
}
```



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:54
   {HighlightingKind::StaticMethod, "StaticMethod"},
+  {HighlightingKind::Typedef, "Typedef"},
+  {HighlightingKind::DependentType, "DependentType"},

thanks for catching this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:181
+  addToken(E->getMemberLoc(), E->getQualifier()
+  ? HighlightingKind::StaticField
+  : HighlightingKind::Field);

nridge wrote:
> hokein wrote:
> > This could be member functions, a case is like
> > 
> > ```
> > template
> > class Foo {
> > public:
> >   void foo() {
> > this->foo();
> >   }
> > };
> > ```
> Thanks for the example.
> 
> Do you have a suggestion for how to discriminate this case? To me, it would 
> seem logical to do it based on syntax (highlight as a member function if the 
> expression forms the function name of a function call expression). That would 
> require navigating from the expression to its parent node. Is there a way to 
> do that?
There is no way to do this in C++.
Even if the name is followed by a pair of parenthese, this could either be a 
field with overloaded `operator()` (e.g. a `std::function field`) or a 
function with the same name.

It's much better to pick a separate highlighting kind for dependent names, this 
follows the actual semantics of C++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Nathan Ridge via Phabricator via cfe-commits]

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

Updated to use a new "dependent type" highlighting


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901

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

Index: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
@@ -51,6 +51,8 @@
   {HighlightingKind::StaticField, "StaticField"},
   {HighlightingKind::Method, "Method"},
   {HighlightingKind::StaticMethod, "StaticMethod"},
+  {HighlightingKind::Typedef, "Typedef"},
+  {HighlightingKind::DependentType, "DependentType"},
   {HighlightingKind::TemplateParameter, "TemplateParameter"},
   {HighlightingKind::Primitive, "Primitive"},
   {HighlightingKind::Macro, "Macro"}};
@@ -175,7 +177,7 @@
   }
   template
   struct $Class[[C]] : $Namespace[[abc]]::$Class[[A]]<$TemplateParameter[[T]]> {
-typename $TemplateParameter[[T]]::A* $Field[[D]];
+typename $TemplateParameter[[T]]::$DependentType[[A]]* $Field[[D]];
   };
   $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Variable[[AA]];
   typedef $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Class[[AAA]];
@@ -509,6 +511,55 @@
   $Typedef[[Pointer]], $Typedef[[LVReference]], $Typedef[[RVReference]],
   $Typedef[[Array]], $Typedef[[MemberPointer]]);
   };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$Function[[bar]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  class $Class[[A]] {
+template 
+$Primitive[[void]] $Method[[bar]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[U]] $Parameter[[P]]) {
+$Class[[A]]().$Method[[bar]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  struct $Class[[A]] {
+template 
+static $Primitive[[void]] $StaticMethod[[foo]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  struct $Class[[B]] {
+$Primitive[[void]] $Method[[bar]]() {
+  $Class[[A]]::$StaticMethod[[foo]]($TemplateParameter[[T]]());
+}
+  };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]](typename $TemplateParameter[[T]]::$DependentType[[Type]]
+= $TemplateParameter[[T]]::$StaticField[[val]]);
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$Parameter[[P]].$Field[[Field]];
+  }
+)cpp",
+  R"cpp(
+  template 
+  class $Class[[A]] {
+$Primitive[[int]] $Method[[foo]]() {
+  return $TemplateParameter[[T]]::$StaticField[[Field]];
+}
+  };
 )cpp"};
   for (const auto  : TestCases) {
 checkHighlightings(TestCase);
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
@@ -41,6 +41,9 @@
 # CHECK-NEXT:"entity.name.type.typedef.cpp"
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
+# CHECK-NEXT:"entity.name.type.dependent.cpp"
+# CHECK-NEXT:  ],
+# CHECK-NEXT:  [
 # CHECK-NEXT:"entity.name.namespace.cpp"
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  [
@@ -61,7 +64,7 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADAA8EAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:],
 # CHECK-NEXT:"textDocument": {
@@ -76,11 +79,11 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 0,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADAA8EAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 1,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADAA8EAAEAAA=="
 # CHECK-NEXT:  }
 # CHECK-NEXT:],
 # CHECK-NEXT:"textDocument": {
@@ -95,7 +98,7 @@
 # CHECK-NEXT:"lines": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"line": 1,
-# CHECK-NEXT:"tokens": "AAADAA4EAAEAAA=="
+# CHECK-NEXT:"tokens": "AAADAA8EAAEAAA=="
 # 

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Nathan Ridge via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:181
+  addToken(E->getMemberLoc(), E->getQualifier()
+  ? HighlightingKind::StaticField
+  : HighlightingKind::Field);

hokein wrote:
> This could be member functions, a case is like
> 
> ```
> template
> class Foo {
> public:
>   void foo() {
> this->foo();
>   }
> };
> ```
Thanks for the example.

Do you have a suggestion for how to discriminate this case? To me, it would 
seem logical to do it based on syntax (highlight as a member function if the 
expression forms the function name of a function call expression). That would 
require navigating from the expression to its parent node. Is there a way to do 
that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

The heuristic of guessing a concrete type seems fragile, and may not work for 
all cases.  Instead of trying out test to guess the underlying type, I think we 
could introduce a new highlighting kind (e.g. `entity.name.type.dependent`) for 
this.




Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:181
+  addToken(E->getMemberLoc(), E->getQualifier()
+  ? HighlightingKind::StaticField
+  : HighlightingKind::Field);

This could be member functions, a case is like

```
template
class Foo {
public:
  void foo() {
this->foo();
  }
};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67901



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

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

[Nathan Ridge via Phabricator via cfe-commits]

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

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67901

Files:
  clang-tools-extra/clangd/SemanticHighlighting.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
@@ -175,7 +175,7 @@
   }
   template
   struct $Class[[C]] : $Namespace[[abc]]::$Class[[A]]<$TemplateParameter[[T]]> {
-typename $TemplateParameter[[T]]::A* $Field[[D]];
+typename $TemplateParameter[[T]]::$Class[[A]]* $Field[[D]];
   };
   $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Variable[[AA]];
   typedef $Namespace[[abc]]::$Class[[A]]<$Primitive[[int]]> $Class[[AAA]];
@@ -509,6 +509,55 @@
   $Typedef[[Pointer]], $Typedef[[LVReference]], $Typedef[[RVReference]],
   $Typedef[[Array]], $Typedef[[MemberPointer]]);
   };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$Function[[bar]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  class $Class[[A]] {
+template 
+$Primitive[[void]] $Method[[bar]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[U]] $Parameter[[P]]) {
+$Class[[A]]().$Method[[bar]]($Parameter[[P]]);
+  }
+)cpp",
+  R"cpp(
+  struct $Class[[A]] {
+template 
+static $Primitive[[void]] $StaticMethod[[foo]]($TemplateParameter[[T]]);
+  };
+
+  template 
+  struct $Class[[B]] {
+$Primitive[[void]] $Method[[bar]]() {
+  $Class[[A]]::$StaticMethod[[foo]]($TemplateParameter[[T]]());
+}
+  };
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]](typename $TemplateParameter[[T]]::$Class[[Type]]
+= $TemplateParameter[[T]]::$StaticField[[val]]);
+)cpp",
+  R"cpp(
+  template 
+  $Primitive[[void]] $Function[[foo]]($TemplateParameter[[T]] $Parameter[[P]]) {
+$Parameter[[P]].$Field[[Field]];
+  }
+)cpp",
+  R"cpp(
+  template 
+  class $Class[[A]] {
+$Primitive[[int]] $Method[[foo]]() {
+  return $TemplateParameter[[T]]::$StaticField[[Field]];
+}
+  };
 )cpp"};
   for (const auto  : TestCases) {
 checkHighlightings(TestCase);
Index: clang-tools-extra/clangd/SemanticHighlighting.cpp
===
--- clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -152,6 +152,38 @@
 return true;
   }
 
+  bool VisitUnresolvedLookupExpr(UnresolvedLookupExpr *E) {
+if (canHighlightName(E->getName())) {
+  addToken(E->getNameLoc(), E->getNamingClass()
+? HighlightingKind::StaticMethod
+: HighlightingKind::Function);
+}
+return true;
+  }
+
+  bool VisitUnresolvedMemberExpr(UnresolvedMemberExpr *E) {
+if (canHighlightName(E->getMemberName())) {
+  addToken(E->getNameLoc(), HighlightingKind::Method);
+}
+return true;
+  }
+
+  bool VisitDependentScopeDeclRefExpr(DependentScopeDeclRefExpr *E) {
+if (canHighlightName(E->getDeclName())) {
+  addToken(E->getLocation(), HighlightingKind::StaticField);
+}
+return true;
+  }
+
+  bool VisitCXXDependentScopeMemberExpr(CXXDependentScopeMemberExpr *E) {
+if (canHighlightName(E->getMember())) {
+  addToken(E->getMemberLoc(), E->getQualifier()
+  ? HighlightingKind::StaticField
+  : HighlightingKind::Field);
+}
+return true;
+  }
+
   bool VisitNamedDecl(NamedDecl *ND) {
 if (canHighlightName(ND->getDeclName()))
   addToken(ND->getLocation(), ND);
@@ -187,6 +219,11 @@
 return true;
   }
 
+  bool VisitDependentNameTypeLoc(DependentNameTypeLoc L) {
+addToken(L.getNameLoc(), HighlightingKind::Class);
+return true;
+  }
+
   bool VisitTypeLoc(TypeLoc TL) {
 if (auto K = kindForType(TL.getTypePtr()))
   addToken(TL.getBeginLoc(), *K);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits