[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 <https://reviews.llvm.org/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<TemplateDecl>(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<HighlightingKind> 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