This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Implement semantic highlightings via findExplicitReferences
ClosedPublic

Authored by ilya-biryukov on Oct 31 2019, 10:05 AM.

Details

Summary

To keep the logic of finding locations of interesting AST nodes in one
place.

The advantage is better coverage of various AST nodes, both now and in
the future: as new nodes get added to findExplicitReferences, semantic
highlighting will automatically pick them up.

The drawback of this change is that we have to traverse declarations
inside our file twice in order to highlight dependent names, 'auto'
and 'decltype'. Hopefully, this should not affect the actual latency
too much, most time should be spent in building the AST and not
traversing it.

Diff Detail

Event Timeline

ilya-biryukov created this revision.Oct 31 2019, 10:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2019, 10:05 AM

Build result: pass - 59818 tests passed, 0 failed and 762 were skipped.
Log files: console-log.txt, CMakeCache.txt

nridge added a subscriber: nridge.Oct 31 2019, 2:01 PM
hokein added inline comments.Nov 4 2019, 2:38 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
195

nit: I'd call it LangOpts

201

maybe call it CollectExtraHiglightings or CollectAdditionalHighlightings?

222

IIUC, if the decls don't have the same kind, we will not highlight it (previously, we fall back to DependentName).

ilya-biryukov marked 2 inline comments as done.
  • Rename to LangOpts and SourceMgr
  • Rename to CollectExtraHighlightings
ilya-biryukov marked 2 inline comments as done.Nov 4 2019, 8:23 AM
ilya-biryukov added inline comments.
clang-tools-extra/clangd/SemanticHighlighting.cpp
222

You're right. I don't think highlighting as dependent name is any better than not highlighting at all in that case, though.
Therefore, I would argue it's not worth adding more code to handle this corner case (I'm not even sure it's possible in practice, it'll take quite some time to come up with examples like these)

Build result: pass - 59845 tests passed, 0 failed and 763 were skipped.
Log files: console-log.txt, CMakeCache.txt

hokein accepted this revision.Nov 5 2019, 7:48 AM
hokein added inline comments.
clang-tools-extra/clangd/SemanticHighlighting.cpp
222

I thought it is better to highlight it rather than not highlighting, but I agree with you, it may be not worth handling this edge case.

This revision is now accepted and ready to land.Nov 5 2019, 7:48 AM
This revision was automatically updated to reflect the committed changes.