This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add missing highlights for using decls.
ClosedPublic

Authored by hokein on Oct 28 2019, 5:43 AM.

Diff Detail

Event Timeline

hokein created this revision.Oct 28 2019, 5:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2019, 5:43 AM

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

ilya-biryukov added inline comments.Oct 28 2019, 6:52 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
51

Could we reuse kindForCandidateDecls instead?
It's best to keep one way to highlight multiple decls.

60

Using decls never have any references and we only highlight them at their declarations locations.
Therefore kindForDecl seems like the wrong place for it. Could we add a VisitUsingDecl method to the visitor and have this logic there instead?

hokein updated this revision to Diff 226673.Oct 28 2019, 8:49 AM
hokein marked 3 inline comments as done.

address comments.

hokein added inline comments.Oct 28 2019, 8:51 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
51

Yes, but note that we need to handle the UsingShadowDecl in this function.

60

good point. done.

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

ilya-biryukov accepted this revision.Oct 28 2019, 9:04 AM

LGTM. See the NITs, though

clang-tools-extra/clangd/SemanticHighlighting.cpp
51

Maybe put this at the first line? This looks like a very good first step.

52

Maybe do D = USD->getTargetDecl() instead?
This would align with how we handle templates in this function and avoid recursive calls (which always look a bit suspicious, e.g. require more thought to ensure the function does not recurse indefinitely)

This revision is now accepted and ready to land.Oct 28 2019, 9:04 AM
hokein updated this revision to Diff 226851.Oct 29 2019, 1:48 AM
hokein marked 2 inline comments as done.

address remaining nits.

This revision was automatically updated to reflect the committed changes.